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

Propose TAP 11 #74

Closed
wants to merge 1 commit into from
Closed

Conversation

heartsucker
Copy link
Contributor

@heartsucker heartsucker commented Sep 14, 2017

Also, as as note, I think using PEM encoding and not just BASE64 is a waste of bytes. That means every key has -----BEGIN PUBLIC KEY----- and -----END PUBLIC KEY----- as well as a few newlines. In a large repo where thousands of keys might be distributed, this adds up. This also only affects text-based formats like JSON or XML.

Copy link
Contributor

@vladimir-v-diaz vladimir-v-diaz left a comment

Choose a reason for hiding this comment

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

@heartsucker
We at NYU are reviewing TAP 11 and giving it some thought. I anticipate others sharing their thoughts and chiming in soon.

My initial reaction is hesitation at having the specification mandate a specific key format, however, others here have not quite dismissed the idea of mandating a specific key format. I personally wonder if it's better for adopters to choose the format of keys listed in metadata.

What are your reasons for requiring PKCS#8, apart from it being a preference?

This review contains my comments of TAP 11 while reading it.

\cc @JustinCappos @awwad @lukpueh @SantiagoTorres @trishankkarthik


All keys have the format:

{ "keytype" : KEYTYPE,
Copy link
Contributor

Choose a reason for hiding this comment

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

This format does not match the one below, which includes an additional field (scheme).


- Redefine how key IDs are calculated
- Specify usage of PKCS#8 encoding and OID for ed25519 keys
- Remote `public` wrapper from `keyval`
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: remove

"keyval" : PUBLIC
}

In both the 'ed25519' and 'rsa' cases, the PUBLIC portion of the key is PKCS#8 encoded and in PEM
Copy link
Contributor

Choose a reason for hiding this comment

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

It's good to use examples with RSA and Ed25519 keys, but I was surprised to see highly specific restrictions with respect to Ed25519's OID. Does it make sense for us to place restrictions for only these two key types, and leave all other key type restrictions to the discretion of adopters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This again comes back to interop. If we want Ed25519 keys to be parseable by many impls, we have to use the same OID.

format. ed25519 keys MUST use the algorithm identifier with OID 1.3.101.112 (curveEd25519) and MUST
NOT use the algorithm identifier with OID 1.2.840.10045.2.1 (ecPublicKey).

The KEYID of a key is the hex digest of the SHA-256 hash of the DER encoded PUBLIC value.
Copy link
Contributor

Choose a reason for hiding this comment

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

This TAP states that TUF is moving towards supporting any metadata format, and argues that specific formats should be avoided so that Key IDs are calculated independent of the metadata format.

I am confused as to why the TAP now requires DER to calculate Key IDs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What other method would be relatively encoding agnostic for hashing to a key ID?

format. ed25519 keys MUST use the algorithm identifier with OID 1.3.101.112 (curveEd25519) and MUST
NOT use the algorithm identifier with OID 1.2.840.10045.2.1 (ecPublicKey).

The KEYID of a key is the hex digest of the SHA-256 hash of the DER encoded PUBLIC value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we require SHA-256? What if we left the choice of hashing algorithm to adopters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't care about interop, then this whole TAP could get torched. If we do, we have to specify the hash.

Copy link

Choose a reason for hiding this comment

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

What if we baked in the hash algorithm into the key id so that it could evolve over time? For example, libsodium's password hashing functions bake in either a $7$ for scrypt, or $argon2id$ for argon2. We could do something similar.

Copy link
Member

Choose a reason for hiding this comment

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

Well this actually would be helpful in being backwards compatible and help libraries figure out the hash algorithm with in-band information.

"keyval" : PUBLIC
}

In both the 'ed25519' and 'rsa' cases, the PUBLIC portion of the key is PKCS#8 encoded and in PEM
Copy link
Contributor

Choose a reason for hiding this comment

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

You made a follow-on comment indicating that PEMs should not be specified in metadata to minimize size. Should the TAP be updated?

@heartsucker
Copy link
Contributor Author

@vladimir-v-diaz

My initial reaction is hesitation at having the specification mandate a specific key format...

We had an internal discussion at work and were using this as a bit of a weather vane to answer the question:

Should TUF completely definite key formats, metadata formats, etc. per encoding (JSON, DER), and should we expect any interoperability between implementations?

Of the code I've looked at (apart from my person stuff and our work stuff), it seems everything copies the python implementation thus making it a de facto standard. In some other discussion I've had with y'all, it seems that answer to the above question is "no, we're not specifying formats," but the community hasn't taken up on that.

If this is the case, I'd say we actually nix this PR and open another one on the main repo that rewrites the spec as something format agnostic.

@JustinCappos
Copy link
Member

JustinCappos commented Sep 26, 2017 via email

@vladimir-v-diaz
Copy link
Contributor

We'd also be interested in hearing from @ecordell @endophage @hannesm.
Which is more preferable, a format agnostic specification or required formats like DER, JSON, PKCS#8, etc? Something else?

@awwad
Copy link
Contributor

awwad commented Sep 26, 2017

I'd like to make sure some things are clear:

Key format specification

  • The TUF Spec does specify the key format, in a section that includes the words "All keys have the format:".
  • Uptane doesn't specify a key format, but the reference implementation uses TUF's format.

Metadata specification

  • The TUF Spec fully defines its metadata format, not quite making it to the level of encoding on the wire, but going so far as to specify canonical JSON.
  • Uptane explicitly does not endorse or specify a metadata format, but examples used in the Implementation Specification are provided in an ASN.1 format.
  • (I still feel like we have to be as agnostic as possible on this in the specification for Uptane, if not TUF.)

As a side-note, the question of whether or not someone's metadata is compatible with the TUF or Uptane reference implementations can be split into two questions:

  • Can your metadata be processed correctly by the TUF reference implementation as-is?
  • Can your metadata be mapped 1-1 automatically to equivalent metadata that can be processed correctly by the TUF reference implementation.

@endophage
Copy link

I don't agree entirely with the motivations, but I do think this is a reasonable and worthwhile effort to increase interoperability. My preference would be for the PKCS#8 in PEM format as proposed, this meets many compliance requirements we see in enterprise.

I would ask though why we don't go a step further than proposed. The keytype field becomes redundant if the type is encoded in an even more specific format in the proposed PEM. Going even further, should we also just move the scheme field in to the PEM and change the definition of the keys maps to KEYID -> PKCS8_PEM, with the KEYID being the sha256 of that binary blob?

It would be necessary (and I think reasonable, this would not be the only place we already do similar at docker), to then also specify clients cannot deserialize and reserialize the PEM unless they also intend to perform the necessary signing for the roles in question. FWIW this general principle has helped us maintain a number of integrity guarantees in docker products even when the specification of a particular data format changes.

@heartsucker
Copy link
Contributor Author

@endophage We actually don't even need the key IDs. The clients have to calculate them locally anyway then verify they match what's in the metadata. We could just send a list of keys and let the clients calculate the key ID locally. If they do it the same way the server does, then there's no reason to include them in the transport encoding.

@vladimir-v-diaz
Copy link
Contributor

@heartsucker I recall you explaining somewhere why a key ID might be unnecessary. Can you provide a link to this explanation so that I can review/think about it again?

A key ID can succinctly identify any type of key data. It is included in the "signatures" field of metadata (example), and to bind keys to roles (example).

If not a keyID, are you suggesting that we instead place the full key data in these cases?

@heartsucker
Copy link
Contributor Author

Here's the discussion: theupdateframework/rust-tuf#118

Key IDs are still used as they are now. Because they are deterministic, both parties will generate the same values from a given key, so they don't need to transport them.


# Motivation

If a TUF respository exsits that serves content in DER format, then all servers and clients must
Copy link

Choose a reason for hiding this comment

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

typo: exsits -> exists.

format. ed25519 keys MUST use the algorithm identifier with OID 1.3.101.112 (curveEd25519) and MUST
NOT use the algorithm identifier with OID 1.2.840.10045.2.1 (ecPublicKey).

The KEYID of a key is the hex digest of the SHA-256 hash of the DER encoded PUBLIC value.
Copy link

Choose a reason for hiding this comment

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

What if we baked in the hash algorithm into the key id so that it could evolve over time? For example, libsodium's password hashing functions bake in either a $7$ for scrypt, or $argon2id$ for argon2. We could do something similar.

@SantiagoTorres
Copy link
Member

@erickt Thanks for bumping this in my list of issues. This is definitely something I'd love to see move forward soon!

@JustinCappos
Copy link
Member

@heartsucker We're likely moving toward moving the specific format outside of the core TUF spec. ( #106 ) Is it okay, if we close this PR and move this issue to the wireline format instead as a result?

@trishankatdatadog
Copy link
Member

Some people, such as Datadog, are using the TUF reference implementation in production. I just saw that the TAPs for pre-release and release schedules were closed without announcements beforehand. Where would backwards-incompatible TAPs like this be implemented? I think we need to answer this question before moving forward.

@JustinCappos
Copy link
Member

TAPs that break backwards compatibility will be implemented in a forthcoming (major version changing) release of TUF.

We'll discuss what to do about posting information about release schedules / project roadmaps. I want to expose this information in the most transparent, helpful way possible. Ideally, I'd like to do something uniform for the lab and many projects do not have a TAP / ITE process or similar to manage forthcoming releases...

@JustinCappos
Copy link
Member

Closing this due to this being superseded by addition of POUFs in #106

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.

None yet

8 participants