Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

in-toto-run implementation #56

Merged

Conversation

shibumi
Copy link
Collaborator

@shibumi shibumi commented Jun 23, 2020

The following PR is part of the Google Summer of Code 2020 program

Fixes issue #54:
This PR intents to fix Issue #54

Description of pull request:
The goal of this PR is the in-toto-run implementation similar to our reference implementation in python.

Please verify and check that the pull request fulfills the following
requirements
:

  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

LoadPublicKey() and VerifySignature() were too generic.
Let's rename them to be more precise in what they are achieving.
@shibumi shibumi marked this pull request as draft June 23, 2020 09:30
@shibumi shibumi changed the title use more specific names intoto-run implementation Jun 23, 2020
In this commit we add a LoadEd25519PublicKey func for loading
ed25519 keys in PrivateJSON format from a ed25519 public key file
@shibumi shibumi force-pushed the shibumi/54-intoto-capturing-implementation branch from 514e090 to 45a8e93 Compare June 23, 2020 11:11
The format of a public key is different
to the private key JSON format for ed25519 in-toto pubkeys.
Therefore we need another function for parsing ed25519 pub keys.
@shibumi shibumi force-pushed the shibumi/54-intoto-capturing-implementation branch 5 times, most recently from 3e13755 to c66ee1f Compare June 23, 2020 13:54
@shibumi shibumi force-pushed the shibumi/54-intoto-capturing-implementation branch from c66ee1f to c6cb9c0 Compare June 23, 2020 13:56
@shibumi
Copy link
Collaborator Author

shibumi commented Jun 23, 2020

@lukpueh current state right now regarding keylib, all including tests:

  • VerifyEd25519
  • ParseEd25519FromPublicJSON
  • LoadEd25519PublicKey
  • LoadEd25519PrivateKey
  • ParseRSAPrivateKeyFromPEM
  • LoadRSAPrivateKey

@shibumi
Copy link
Collaborator Author

shibumi commented Jun 23, 2020

I've also added a bob.pub and bob-invalid.pub test file for testing reading from a file. I took a real file generated by in-toto-keygen -p -t ed25519 bob (I hope this is correct and the format is correct).

@lukpueh
Copy link
Member

lukpueh commented Jun 24, 2020

@lukpueh current state right now regarding keylib, all including tests:
Thanks for the heads-up and for working on a draft PR. I appreciate your transparent work style! Let me know if you want me to look at anything in particular.

I've also added a bob.pub and bob-invalid.pub test file for testing reading from a file. I took a real file generated by in-toto-keygen -p -t ed25519 bob (I hope this is correct and the format is correct).

Yes, this is what I would have done, as it gives us a sense for interoperability.

@shibumi
Copy link
Collaborator Author

shibumi commented Jun 24, 2020

Thanks for the feedback. I am going to finish the missing functions till Friday, then I will start working on the link meta data creation and I hope that I can generate the first link meta data before Monday :)
If this works (including testing, good coverage, etc), we can have a look on interfacing a gpg agent or RecordStart() RecordStop() functionality.

EDIT: If we continue with this pace we can maybe finish the runtime before August and I can have a look on the exclude PR again or any other todo in our README. :)

This commit adds more test material such like symmetric encrypted private keys.
This commit also changes the example keys in the documentation.
We use the keypair of "carol" (see test/data/carol{.pub}) now.
@shibumi
Copy link
Collaborator Author

shibumi commented Jun 26, 2020

Right now loading encrypted private keys is not yet supported. I need to have a look at it, later.
The securesystemslib says we are using AES-256-CTR for key encryption:

https://github.com/secure-systems-lab/securesystemslib/blob/01a0c95af5f458235f96367922357958bfcf7b01/securesystemslib/keys.py#L1309

not sure how easy we can implement key encryption/decyption in in-toto-golang.

We use PKCS1 for parsing/loading RSA private keys.
This means we do not support ECDSA yet.
@shibumi
Copy link
Collaborator Author

shibumi commented Jun 26, 2020

@lukpueh Do we need generate keys in in-toto-golang?

@shibumi shibumi force-pushed the shibumi/54-intoto-capturing-implementation branch from 14fad67 to 849dddb Compare June 26, 2020 08:50
@shibumi shibumi force-pushed the shibumi/54-intoto-capturing-implementation branch from 849dddb to 9b5413f Compare June 26, 2020 08:53
@lukpueh
Copy link
Member

lukpueh commented Jun 26, 2020

@lukpueh Do we need generate keys in in-toto-golang?

I'd say no, as long we can parse keys from standard formats. It actually seems preferable to support interoperability with well-known tool chains, such as openssl or gpg, for key creation (see secure-systems-lab/securesystemslib#55 for additional infos).

@shibumi
Copy link
Collaborator Author

shibumi commented Jun 26, 2020

@lukpueh k, then we should have all necessary functionality right now for signing/loading keys. Except for ecdsa support + support for encrypted keys.

@lukpueh
Copy link
Member

lukpueh commented Jun 26, 2020

Awesome, I'll take a closer look early next week. :)

@shibumi
Copy link
Collaborator Author

shibumi commented Jun 26, 2020

I correct: The RSA Sign method is missing. I am coding on this one right now. I just need to clarify one question I have about rsa.SignPSS I have no idea if we can pass nil as random source here.. normally Go should fall back to a sane default, but I had a look on the decryptAndCheck function in rsa.SignPSS and then on the decrypt function and I don't see any comment that is mentioning what happens when rand == nil..

I've asked one of the Go Core Cryptographers, he helped me in the past ^^ so maybe he can give me a hint, before I do something totally insane. https://twitter.com/Sh1bumi/status/1276463063898583046

We have a few default cases, that were never reached due to
validateKey() at the beginning of the function.
Let's call a panic, if we run into such a situation.
This commit addresses various spelling issues, substantial mistakes
or adds more documentation.
This adds a short comment/todo to our subsetCheck function.
In the future we might want to use Sets for our constant getters.
This commits removes the misleading support for ecdsa-sha2-nistp384.
with this commit implements the validatePublicKey function, for
checking if we deal with a public key. If the private key value field
is not empty, it will fail with the error ErrNoPublicKey.
We also call this method in validateLayout from now on.
@shibumi
Copy link
Collaborator Author

shibumi commented Aug 17, 2020

It's funny that our test coverage decreased, only because of the additional panic + comments. Do comments count as coverage? I can't explain it otherwise. I will try to add a few tests for not catched errors, but it's getting more and more difficult, because most errors are either library external (I don't want to test against functions like ioutil.Writer...), or code that we will never reach (no idea how to test, such code).

If you want to have a look on the current coverage in a nice graphical way. Have a look at the coverprofile via:

$ go test -coverprofile=coverage.out ./... && go tool cover -html=coverage.out"

this should spawn a new tab in your browser with the coverage highlighted on the code in green and red.

@lukpueh
Copy link
Member

lukpueh commented Aug 17, 2020

mhh not sure if a ticket is the right place for this. I suggest adding a comment below/before the key constants. A ticket is for me an actionable, something that needs to be resolved or changed. We can't really do something about this, until it's changed in the securesystemslib, right?

Agreed. Although, it would be nice if the comment were somewhere on the public interface. E.g. on LoadKey (and/or {Generate,Verify}Signature. What do you think?

@lukpueh
Copy link
Member

lukpueh commented Aug 17, 2020

... it's getting more and more difficult, because most errors are either library external (I don't want to test against functions like ioutil.Writer...), or code that we will never reach (no idea how to test, such code).

Don't worry about those.

@shibumi
Copy link
Collaborator Author

shibumi commented Aug 17, 2020

Agreed. Although, it would be nice if the comment were somewhere on the public interface. E.g. on LoadKey (and/or {Generate,Verify}Signature. What do you think?

makes sense, let me start GoLand and add a comment for those functions :)

In this commit, we make Key.KeyIdHashAlgorithms optional.
We only check the field now, if the field has been initialized.
Furthermore this commit fixes a few tests and removes tests, that are
not needed anymore.
in-toto-golang behaves a little bit different to the securesystemslib.
We should mention, that we use ecdsa/ecdsa-sha2-nistp256 pairs
instead of ecdsa-sha2-nistp256 for key type and key scheme.
@shibumi
Copy link
Collaborator Author

shibumi commented Aug 17, 2020

mhh not sure if a ticket is the right place for this. I suggest adding a comment below/before the key constants. A ticket is for me an actionable, something that needs to be resolved or changed. We can't really do something about this, until it's changed in the securesystemslib, right?

Agreed. Although, it would be nice if the comment were somewhere on the public interface. E.g. on LoadKey (and/or {Generate,Verify}Signature. What do you think?

Is this okay? 1ac47be

This adds the missing 'd' to the deadbeef test. We now check for a missing
keyfield there.
Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing left to do here! 🎉 😄

... except for maybe removing the some items from the TODO list in the main REAMDE file.

in-toto-golang now supports all signing methods from the reference
implementation and has a fully-fledged runlib, to generate signed
link metadata.

Big kudos to @shibumi!

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
@lukpueh lukpueh marked this pull request as ready for review August 20, 2020 12:41
@lukpueh lukpueh merged commit 4aecd39 into in-toto:master Aug 20, 2020
@shibumi shibumi deleted the shibumi/54-intoto-capturing-implementation branch August 20, 2020 14:16
@lukpueh lukpueh mentioned this pull request Oct 7, 2020
2 tasks
adityasaky added a commit that referenced this pull request Sep 17, 2021
@adityasaky adityasaky mentioned this pull request Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants