Make ECDSA key parsing more flexible #426

flavio · 2021-10-27T16:47:58Z

NOTE WELL: this PR is built on top of #425

TUF specification suggests to encode ECDSA keys using PEM. This is the behaviour of the python reference implementation.

However, go-tuf encodes the keys as Hex numbers.

This commit makes the decoding of ECDSA keys more relaxed, the code will try to decode them as PEM strings and it will resort to Hex when the first decode fails.

flavio · 2021-10-27T17:03:59Z

The code works fine against Sigstore's root.json, meaning it's not generating an error while parsing the file. However, none of the 5 keys defined inside of the root.json file can be used to verify its contents 🤔

I tried to understand what is going on, but I couldn't understand it :(

I'm using a local build of the tuftool to download the repository:

tuftool download \
   -m https://sigstore-tuf-root.storage.googleapis.com \
   -t https://sigstore-tuf-root.storage.googleapis.com/targets \
   --allow-root-download \
   sigstore-tuf-root

trishankatdatadog · 2021-10-27T17:11:14Z

tough/src/schema/decoded.rs

+
+impl Decode for EcdsaFlex {
+    fn decode(s: &str) -> Result<Vec<u8>, Error> {
+        match spki::decode(


I would do stricter pattern-matching to be safe:

If I seem the PEM header, I will decode PEM. If I see the hex encoding, I will decode it as such. Otherwise, error out. Makes sense?

Yes, I was just trying to get something quick put together... My goal would be to understand why none of the keys can successfully verify the contents of the file :(

trishankatdatadog · 2021-10-27T17:12:49Z

The code works fine against Sigstore's root.json, meaning it's not generating an error while parsing the file. However, none of the 5 keys defined inside of the root.json file can be used to verify its contents 🤔

Great start, thank you! Not sure why the signature verification is failing, but I'm sure it's missing some cryptographic parameters. Have you tried looking at the go-tuf implementation?

flavio · 2021-10-28T15:04:04Z

tough uses the ring crate to perform verification. This crate doesn't provide fine tuning of the different parameters, it's pretty straightforward.

I've made the following experiment:

I created a small Rust program, that uses ring to create a ECDSA keypair, sign a message with that and print the public key, signature and message on the stdout
I create a Go program that uses the same code as the one you linked above to verify the signature

Everything worked as expected, the Go program was able to verify the signature produced by the Rust library. If you want I can share the code of the programs.

Now, I know this testing something slightly different: we should produce a signature with Go, verify that with Rust. However I didn't have the time to look into how go-tuf is producing the signature.

However, I find reassuring to see everything work as expected out of the box.

I'm getting really lost on this issue, any help? 🙏

trishankatdatadog · 2021-10-28T15:10:23Z

I'm sure it's something really small/silly, but I don't have enough time to look into this. Let me see what @asraa and I can do with some downtime...

webern · 2021-10-28T17:31:56Z

Thank you for working on this!

Are we sure that eagerly trying both encodings does not introduce a security risk?

For this sort of interoperability functionality it would be great to see a test repository, created by the incompatible go-tuf implementation, added to here https://github.com/awslabs/tough/tree/develop/tough/tests/data and a test added here: https://github.com/awslabs/tough/blob/develop/tough/tests/interop.rs

This might aide your development cycles as well since it should serve as a red/green testing scenario.

trishankatdatadog · 2021-10-29T15:49:54Z

Are we sure that eagerly trying both encodings does not introduce a security risk?

Agreed, which is why I recommended that we try only one encoding depending on the format.

asraa · 2021-11-02T14:59:41Z

I just tried the reverse check, where I created a ecdsa signature with go-tuf (underlyingly using crypto/ecdsa SignASN1) and it verified properly with ring::signature, the code here.

I'm skeptical now that the message isn't being canonicalized the same way before checking signature validation.

flavio · 2021-11-02T15:04:32Z

Thanks for having looked into that!

I'm skeptical now that the message isn't being canonicalized the same way before checking signature validation.

I'm starting to have doubts too about that. I've recently used the olpc-cjson crate of tough while verifying a cosign bundle returned by Rekor and it worked. Dunno if Rekor is using the same canonicalization code as go-tuf...

asraa · 2021-11-02T15:06:31Z

while verifying a cosign bundle returned by Rekor and it worked

Nope, the bundle is just a normal marshalled json struct. The TUF code is using a cjson canonicalizer package, for the metadata

Edit: the easiest way for me to test this is to disable any funky indentations we do to make the json nice and see if tuftool can handle a go-tuf generated manifest

flavio · 2021-11-02T15:09:49Z

Nope, the bundle is just a normal marshalled json struct. The TUF code is using a cjson canonicalizer package, for the metadata

Yes, but then cosign internally converts it to cjson before verifying the signature (https://github.com/sigstore/cosign/blob/main/pkg/cosign/verify.go#L335)

flavio · 2021-11-02T15:10:53Z

Edit: the easiest way for me to test this is to disable any funky indentations we do to make the json nice and see if tuftool can handle a go-tuf generated manifest

Cool, I'm totally new to TUF and its tooling. It would be great if you could look into that. If not, let me know and I'll experiment :)

trishankatdatadog · 2021-11-02T15:35:41Z

Dunno if Rekor is using the same canonicalization code as go-tuf...

Probably not

flavio · 2021-12-21T10:11:28Z

@trishankatdatadog, @asraa : I finally figured out what is causing tough to choke on Sigstore's TUF reposotory.

The root file is violating the TUF specification. All the date-time objects have been serialized using RFC 3339, which includes the timezone information.
The TUF specification clearly states:

Metadata date-time follows the ISO 8601 standard. The expected format of the combined date and time string is "YYYY-MM-DDTHH:MM:SSZ". Time is always in UTC, and the "Z" time zone designator is attached to indicate a zero UTC offset. An example date-time string is "1985-10-21T01:21:00Z".

This is what the Sigstore TUF repository root file has inside of it:

"expires": "2021-12-18T13:28:12.99008-06:00"

The tough crate deserializes this data into a series of Rust structs, which are rightfully using a DateTime<Utc> as internal representation. During this process the Timezone information is lost.

Later, the Rust structs are serialized, during this step the date time object becomes:

 "expires": "2021-12-18T19:28:12.990080Z",

The document produced during this process is then used to verify the validity of the signatures of the trusted keys.

The document signed by the Sigstore maintainers is different from the one the Rust client verifies due to this different formatting of the date-time objects. As a result, none of the signatures is verified -> the download stops.

I did a quick fix to address this issue (see commit f06b7a2). With this patch I can successfully use the tuftool to download the Sigstore TUF repository:

tuftool download -m 'https://sigstore-tuf-root.storage.googleapis.com/' -t 'https://sigstore-tuf-root.storage.googleapis.com/targets' --allow-root-download sigstore-tuf-root`
=================================================================
WARNING: Downloading root.json to /home/flavio/hacking/sigstore/tough/tuftool/1.root.json
This is unsafe and will not establish trust, use only for testing
=================================================================
Downloading targets to "sigstore-tuf-root"
	-> fulcio_v1.crt.pem
	-> ctfe.pub
	-> fulcio.crt.pem
	-> artifact.pub
	-> rekor.pub

However, I think my fix is not needed. The problem must be fixed inside of the Sigstore TUF repository.

This PR now includes two commits:

07228f2 - we can address the review feedback expressed above OR we can make the TUF specification more explicit about how ECDSA keys must be encoded (right now there's just a recommendation) and fix Sigstore's TUF repository metadata accordingly
f06b7a2 - I would not merge it because the TUF specification is clear about how datetime objects should be formatted. The issue has to be fixed inside of Sigstore's TUF repository

asraa · 2021-12-21T20:47:07Z

However, I think my fix is not needed. The problem must be fixed inside of the Sigstore TUF repository.

Joshua also pointed this out, this was fixed in the next v2 root (see https://github.com/sigstore/root-signing/blob/9a627a0fc51c202868e0ea10b74007fa4b9e9416/repository/repository/2.root.json#L27).

Why does expiration verification need to happen on 1.root.json?

Also I'm sorry that you were still working on this! It turns out that also the signature verification was fixed in v2 and other issues for tuftool sigstore/root-signing#51

EDIT: I think we will always need to pin trust on the initial root being 2.root.json for rust since the sign verification won't work anyway on 1.root.json. This works without any patch when I was testing v2 root signing.

tuftool download -m 'https://sigstore-tuf-root.storage.googleapis.com/' -t 'https://sigstore-tuf-root.storage.googleapis.com/targets' --root 2.root.json test/
Downloading targets to "test/"

trishankatdatadog · 2021-12-21T22:15:33Z

Why does expiration verification need to happen on 1.root.json?

Correct: you should ignore expiration of any interim roots...

trishankatdatadog · 2021-12-21T22:16:20Z

EDIT: I think we will always need to pin trust on the initial root being 2.root.json for rust since the sign verification won't work anyway on 1.root.json. This works without any patch when I was testing v2 root signing.

Agree: this is the simplest fix for now: to bundle v2 with any tough-based client that will fetch/verify targets.

webern · 2021-12-22T00:05:02Z

the TUF specification is clear about how datetime objects should be formatted

I wonder if we are still going to have issues based on different precisions in fractions of a second, e.g.

"expires": "2021-12-18T19:28:12.990080Z"
"expires": "2021-12-18T19:28:12.9900800Z"

What you quoted from the specification might imply that fractions of a second must not be included, but it doesn't say that explicitly.

I think it makes sense to remove the time serialization changes from this PR and to focus on the ECDSA decoding issue. This may preclude us from adding an interoperability test as I recommended in this PR until the time format issues are addressed.

trishankatdatadog · 2021-12-22T01:19:26Z

What you quoted from the specification might imply that fractions of a second must not be included, but it doesn't say that explicitly.

Might be a good idea to clarify this in the spec either way. PRs are welcome!

flavio · 2022-01-11T16:23:15Z

Hi everybody, I'm back from the End Year holidays and I'm ready to get back to this issue 💪

I've clarified the situation with @trishankatdatadog. The date format issues inside of the Sigstore TUF repositories have been sorted out (thanks @asraa !).
That means there's just one issue to be addressed: make the parsing of the ECDSA keys more flexible.

My initial code works, but as @trishankatdatadog and @webern pointed out (see [1] and [2]), we should do things in a more secure way.

Both @trishankatdatadog and @webern suggested to check the string and:

If the PEM header is found -> handle it as PEM format
If the string is hexadecimal -> handle as Hex format
Otherwise just report an error

My initial thoughts have been:

To check if the string is hexadecimal: validate the string using a regular expression
To check if the string is PEM encoded: use Rust's str.starts_with and look for the -----BEGIN string

I'm not convinced by both approaches. I fear they might not be secure... Do you have better recommendations?

I could also change the current code like that:

Attempt to decode the input string assuming it's PEM encoded
Check the error type raised by the function
- if it's a PEM encoding error then do some other code (attempt a straight hex conversion, or do hex conversion only after we know for sure that the input is hexadecimal)
- If it's not a PEM encoding error -> bubble up the error

Once this aspect is clarified I'll go ahead and implement the new code and extend the test suite as recommended by @webern

webern · 2022-01-11T17:05:09Z

To check if the string is hexadecimal: validate the string using a regular expression

This might not be necessary. Maybe you could do the starts_with check for PEM, and if it's not PEM then assume it is hexadecimal. Something didn't sit right with me (earlier) about trying, failing, and trying the other. But once you've determined that it's not PEM, we can assume it is hexadecimal and it won't parse as such if it would have failed a regex anyway.

flavio · 2022-01-12T15:00:19Z

@webern thanks for the feedback. I've updated the PR to reflect what you suggested. I've also added a new unit test covering this specific case.

webern

The first thing I noticed is that, due to our trait definition, the information about what encoding was encountered is lost. This got me thinking about whether there is a general change that should be made to the trait to describe what encoding format was encountered. This led me to realize I don't know much about key encoding formats so I started poking around...

Then I thought, well I need to look at the spec, and... uh oh, It appears that:

ed25519: PUBLIC :: 64-byte hex encoded string.
ecdsa-sha2-nistp256: PUBLIC :: PEM format and a string.

If I'm understanding things correctly, that means the root.json from your test case is not valid per the spec because it states ecdsa-sha2-nistp256 and uses a hex encoding.

I apologize for missing this earlier, but are we trying to make the tough library accept root.json files that are not formed correctly per the specification?

Here's the section that I'm looking at:

https://github.com/theupdateframework/specification/blob/c43045a85a637a5e2922af2dc1af08691e496780/tuf-spec.md#file-formats-general-principles--file-formats-general-principles

I guess the spec also says that

adopters can define and use any particular keytype, signing scheme, and cryptographic library

but I would interpret that to mean that you can create new keytype strings, not that you can use an existing keytype differently than what the spec defines.

Help me out here if I'm behind the curve! Thanks.

tough/src/schema/decoded.rs

trishankatdatadog · 2022-01-13T00:47:52Z

I guess the spec also says that

adopters can define and use any particular keytype, signing scheme, and cryptographic library

but I would interpret that to mean that you can create new keytype strings, not that you can use an existing keytype differently than what the spec defines.

No, it means that you are free to redefine to use different schemas for the proposed keytype-s.

If this is unclear or undesirable, please do open a PR upstream (cc @lukpueh @joshuagl)

flavio · 2022-01-18T15:58:26Z

If I'm understanding things correctly, that means the root.json from your test case is not valid per the spec because it states ecdsa-sha2-nistp256 and uses a hex encoding.

Good catch, I didn't notice that.

I apologize for missing this earlier, but are we trying to make the tough library accept root.json files that are not formed correctly per the specification?

That was not my intention.

Help me out here if I'm behind the curve! Thanks.

As @trishankatdatadog suggested, I've submitted theupdateframework/specification#201 to shed some light on that.

webern · 2022-01-21T01:19:55Z

If I'm understanding things correctly, that means the root.json from your test case is not valid per the spec because it states ecdsa-sha2-nistp256 and uses a hex encoding.

Good catch, I didn't notice that.

I apologize for missing this earlier, but are we trying to make the tough library accept root.json files that are not formed correctly per the specification?

That was not my intention.

Actually per discussion in theupdateframework/specification#201, it sounds like the spec allows for your root.json to be considered valid. Did you create it with go-tuf? In general, is it go-tuf that we aim to interoperate with by making these changes?

I'm in the cloud-native.slack.com as @Matt Briggs if you want to DM me on this. The main thing missing from my brain with your implementation is what happens when we re-serialize that root.json. Do we encode the key back as a PEM even though when we deserialized it was hex? I can't quite see the answer to that question by inspecting the traits, so I'm going to check out your branch and try it to see what happens.

webern · 2022-01-21T01:47:24Z

Ok, this did at least reserialize the keys in their original hex encoding. I think I want to take it further though and have an example repository created by go-tuf, with targets and metadata, that we can load, add a target to, re-sign, then load with go-tuf. i.e. true interop. Let's chat about it via DM soon.

fn hex_encoded_ecdsa_sig_keys() {
    let path = test_data()
        .join("hex-encoded-ecdsa-sig-keys")
        .join("root.json");
    let mut f = File::open(path).unwrap();

    let root: Signed<Root> = serde_json::from_reader(f).unwrap();
    println!("{}", serde_json::to_string_pretty(&root).unwrap())
}

webern

This looks good @flavio, sorry it took me so long. The only recommended changes are to document the test's purpose and to re-use EcdsaPem::decode in EcdsaFlex::decode

tough/src/schema/decoded.rs

tough/src/schema/de.rs

webern · 2022-01-25T03:17:16Z

Needs a git rebase develop to pick up some clippy fixes.

flavio · 2022-01-25T17:43:25Z

@webern I've rebased my PR against develop. The PR is now ready for review

TUF specification suggests to encode ECDSA keys using PEM. This is the behaviour of the python reference implementation. However, `go-tuf` encodes the keys as Hex numbers. This commit makes the decoding of ECDSA keys more relaxed, the code will try to decode them as PEM strings and it will resort to Hex when the first decode fails. Signed-off-by: Flavio Castelli <[email protected]> Co-authored-by: Matthew James Briggs <[email protected]>

webern

Nice 🚀

zmrow

Thanks again for all the work on this!

👍

trishankatdatadog

LGTM, thanks!

jku · 2022-02-12T08:49:50Z

What you quoted from the specification might imply that fractions of a second must not be included, but it doesn't say that explicitly.

Spec is explicit about time formats ( this is not in the expiry sections themselves but it is in there):

Metadata date-time follows the ISO 8601 standard. The expected format of the combined date and time string is "YYYY-MM-DDTHH:MM:SSZ". Time is always in UTC, and the "Z" time zone designator is attached to indicate a zero UTC offset. An example date-time string is "1985-10-21T01:21:00Z".

This is very clear: timezones and microseconds are not allowed

webern · 2022-02-13T03:51:41Z

This is very clear: timezones and microseconds are not allowed

I created #447 for this.

flavio mentioned this pull request Oct 27, 2021

Fix parsing of root.json file when ecdsa-sha2-nistp256 keys are used #425

Merged

tjkirch marked this pull request as draft October 27, 2021 17:04

trishankatdatadog suggested changes Oct 27, 2021

View reviewed changes

flavio mentioned this pull request Nov 2, 2021

Provide way to download rekor public key from Sigstore TUF repository sigstore/sigstore-rs#9

Closed

flavio force-pushed the ecdsa-flexible-parsing branch from 2ebe167 to f06b7a2 Compare December 21, 2021 09:56

flavio force-pushed the ecdsa-flexible-parsing branch from f06b7a2 to 2fb2030 Compare January 12, 2022 14:59

webern reviewed Jan 12, 2022

View reviewed changes

tough/src/schema/decoded.rs Outdated Show resolved Hide resolved

tough/src/schema/decoded.rs Show resolved Hide resolved

flavio mentioned this pull request Jan 18, 2022

Clarify key format theupdateframework/specification#201

Open

webern reviewed Jan 25, 2022

View reviewed changes

tough/src/schema/decoded.rs Show resolved Hide resolved

tough/src/schema/de.rs Show resolved Hide resolved

flavio force-pushed the ecdsa-flexible-parsing branch from 2fb2030 to 5ad6f66 Compare January 25, 2022 17:42

flavio marked this pull request as ready for review January 25, 2022 17:42

flavio force-pushed the ecdsa-flexible-parsing branch from d6eb9ba to b6e4c0f Compare January 26, 2022 15:31

flavio force-pushed the ecdsa-flexible-parsing branch from b6e4c0f to eb1bdb8 Compare January 26, 2022 15:52

webern approved these changes Jan 26, 2022

View reviewed changes

zmrow approved these changes Jan 26, 2022

View reviewed changes

trishankatdatadog approved these changes Jan 26, 2022

View reviewed changes

webern merged commit d9f0cb8 into awslabs:develop Jan 26, 2022

webern mentioned this pull request Jan 27, 2022

release #444

Merged

jku mentioned this pull request Feb 12, 2022

compatibility: go-tuf ecdsa key format is incompatible theupdateframework/python-tuf#1859

Closed

webern mentioned this pull request Feb 13, 2022

tough: dates with microseconds may be non-compliant #447

Open

flavio deleted the ecdsa-flexible-parsing branch February 14, 2022 08:09

asraa mentioned this pull request Jul 21, 2022

bug: python-tuf incompatibility because of hex-encoded ECDSA keys sigstore/root-signing#329

Closed

Make ECDSA key parsing more flexible #426

Make ECDSA key parsing more flexible #426

Conversation

flavio commented Oct 27, 2021

flavio commented Oct 27, 2021

trishankatdatadog Oct 27, 2021

Choose a reason for hiding this comment

flavio Oct 27, 2021

Choose a reason for hiding this comment

trishankatdatadog commented Oct 27, 2021

flavio commented Oct 28, 2021

trishankatdatadog commented Oct 28, 2021

webern commented Oct 28, 2021

trishankatdatadog commented Oct 29, 2021

asraa commented Nov 2, 2021

flavio commented Nov 2, 2021

asraa commented Nov 2, 2021 • edited Loading

flavio commented Nov 2, 2021

flavio commented Nov 2, 2021

trishankatdatadog commented Nov 2, 2021

flavio commented Dec 21, 2021

asraa commented Dec 21, 2021 • edited Loading

trishankatdatadog commented Dec 21, 2021

trishankatdatadog commented Dec 21, 2021

webern commented Dec 22, 2021

trishankatdatadog commented Dec 22, 2021

flavio commented Jan 11, 2022

webern commented Jan 11, 2022 • edited Loading

flavio commented Jan 12, 2022

webern left a comment • edited Loading

Choose a reason for hiding this comment

trishankatdatadog commented Jan 13, 2022

flavio commented Jan 18, 2022

webern commented Jan 21, 2022 • edited Loading

webern commented Jan 21, 2022

webern left a comment

Choose a reason for hiding this comment

webern commented Jan 25, 2022

flavio commented Jan 25, 2022

webern left a comment

Choose a reason for hiding this comment

zmrow left a comment

Choose a reason for hiding this comment

trishankatdatadog left a comment

Choose a reason for hiding this comment

jku commented Feb 12, 2022 • edited Loading

webern commented Feb 13, 2022

asraa commented Nov 2, 2021 •

edited

Loading

asraa commented Dec 21, 2021 •

edited

Loading

webern commented Jan 11, 2022 •

edited

Loading

webern left a comment •

edited

Loading

webern commented Jan 21, 2022 •

edited

Loading

jku commented Feb 12, 2022 •

edited

Loading