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

Clarify current state of key formats in the spec #272

Merged
merged 5 commits into from
May 15, 2023

Conversation

joshuagl
Copy link
Member

@joshuagl joshuagl commented Mar 2, 2023

Key formats are a recurring source of confusion (see secure-systems-lab/securesystemslib#513, #201, and secure-systems-lab/securesystemslib#308). Do we specify key formats, or simply record those used by the reference implementation?

The current status is unclear, so this PR attempts to:

  1. clarify that the spec only documents the key formats used by the reference implementation
  2. recommend against redefining the formats documented in the spec
  3. renames the "ecdsa-sha2-nistp256" key format to "ecdsa", per the reference implementation (Better distinguish between keytype and scheme for ECDSA keys secure-systems-lab/securesystemslib#267)

The above changes are included in a single commit ac9fc63.

There are additional changes in this PR to address a bikeshed error in newer versions of the generator (2de4149) and to add additional navigation headers to make it easier to find and link to details of the recommended file formats (5fef375). I included these changes in one PR because the workflow is awkward for small changes.

I encourage reviewers to use the commits tab and review each patch independently.

(Note: deeper discussions around key formats and specification vs. recommendation and interoperability of implementations is necessary, and has been added to the agenda

lukpueh
lukpueh previously approved these changes Mar 14, 2023
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.

Thanks for the patch, @joshuagl! I think this is the right solution for ecdsa keys. Also thanks for structuring the commits in a meaningful way!

Joshua, did you coordinate with @trishankatdatadog that he syncs your branch with upstream?

@joshuagl
Copy link
Member Author

I just rebased on main and pushed an additional commit which: adds a header for the object format and makes that section, the key format section and the date-time section all subsections (one level of heading lower) of the "File formats: general principles" section.

@lukpueh PTAL at 9fe0b01 and re-approve if you agree.

Joshua, did you coordinate with @trishankatdatadog that he syncs your branch with upstream?

We did not coordinate.

@trishankatdatadog
Copy link
Member

Sorry, what branch upstream?

@lukpueh
Copy link
Member

lukpueh commented Mar 16, 2023

Sorry, what branch upstream?

You seemed to have pushed a merge commit (e83ed23) onto Joshua's branch.

joshuagl added 5 commits May 12, 2023 12:31
We claim that the spec is just documenting the signature _schemes_ from
the reference implementation, but that we define three _keytypes_ within
the spec. This change first updates the _keytypes_ to match the reference
implementation (we have defaulted to a generic "ecdsa" keytype since
secure-systems-lab/securesystemslib#267).

Further, we update the specification to clarify that within we are
documenting the keytypes and schemes from the reference implementation,
and that we recommend implementing these keytypes and schemes as specified.

Signed-off-by: Joshua Lock <[email protected]>
Signed-off-by: Joshua Lock <[email protected]>
Make it easier to find key formats and, more importantly because it's
often missed, date-time recommendations.

Signed-off-by: Joshua Lock <[email protected]>
Signed-off-by: Joshua Lock <[email protected]>
These all form part of the general principles of the pedagogical file
format, so move them to be one level heading lower.

Signed-off-by: Joshua Lock <[email protected]>
@joshuagl joshuagl force-pushed the joshuagl/keytype branch from 9fe0b01 to de6b164 Compare May 12, 2023 11:32
@joshuagl
Copy link
Member Author

Rebased on master and force pushed. That didn't dismiss @lukpueh's approval 😌 has GitHub evolved?

@trishankatdatadog @mnm678 any chance of a review/2nd approval?

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.

4 participants