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.

Sign up for GitHub

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

Jump to bottom

Update SigningConfig to specify API version and validity periods #539

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update SigningConfig to specify API version and validity periods #539

wants to merge 1 commit into from

Conversation

haydentherapper
Copy link
Collaborator

In order to faciliate clients gracefully handling breaking API changes, the SigningConfig will now include API versions for each of the service URLs so that clients can determine what services they are compatible with. Additionally, we've included validity periods which will be used to faciliate Rekor log sharding, when we spin up new log shards and distribute new key material.

Fixes #474

Summary

Release Note

Documentation

@haydentherapper
Update SigningConfig to specify API version and validity periods
dbd11fe 
In order to faciliate clients gracefully handling breaking API
changes, the SigningConfig will now include API versions for each
of the service URLs so that clients can determine what services
they are compatible with. Additionally, we've included validity periods
which will be used to faciliate Rekor log sharding, when we spin up new
log shards and distribute new key material.

Fixes sigstore#474

Signed-off-by: Hayden Blauzvern <[email protected]>
@haydentherapper haydentherapper mentioned this pull request Feb 14, 2025
Open
@haydentherapper
Copy link
Collaborator Author

A few things to note:

  • I kept the old url fields because I'm not sure if that will break sigstore-python, which supports ClientTrustConfig.
  • That also means we need a new media type version, which I added.
  • I've included comments for the selection process. For CA and OIDC URLs, the client must select one URL that matches its criteria. For Rekor and TSA URLs, the client must select all URLs that match its criteria. If this is confusing, lemme know and we can think about grouping things differently.
  • Naming is hard because we've already used the abbreviated names I would have used for the pluralized fields....so the names are a bit verbose.

kommendorkapten
kommendorkapten approved these changes Feb 14, 2025
View reviewed changes
Copy link
Member

@kommendorkapten kommendorkapten left a comment

Choose a reason for hiding this comment

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

Looks good!

cmurphy
cmurphy reviewed Feb 14, 2025
View reviewed changes
protos/sigstore_trustroot.proto
Comment on lines +210 to +213
// Clients must select only Service with the highest API version
// that the client is compatible with and that is within its
// validity period. Clients should select the first Service
// that meets this requirement.
Copy link

Choose a reason for hiding this comment

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

If the sort order is the same as for TrustedRoot

protobuf-specs/protos/sigstore_trustroot.proto

Lines 108 to 109 in e943ce1

// All the listed instances SHOULD be sorted by the 'valid_for' in ascending
// order, that is, the oldest instance first. Only the last instance is
then this could result in a client choosing a service that is valid but about to expire, when a later service in the list would be valid for longer.

protos/sigstore_trustroot.proto
// These URL **MUST** be the "base" URLs for the transparency logs,
// which clients should construct appropriate API endpoints on top of.
//
// Clients must select ALL Services with the highest API version
Copy link

Choose a reason for hiding this comment

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

This seems like a big change, as clients don't currently upload entries to multiple logs at once, and may not always want to. Could this be changed to ANY?

protos/sigstore_trustroot.proto
// that the client is compatible with and that is within its
// validity period. Clients should select the first Service
// that meets this requirement.
repeated Service certificate_authority_urls = 6;
Copy link

Choose a reason for hiding this comment

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

I wish the new field names were more meaningfully different from the old names but I don't have a great suggestion.

jku
jku reviewed Feb 18, 2025
View reviewed changes
protos/sigstore_trustroot.proto
Comment on lines +151 to +152
// MUST be application/vnd.dev.sigstore.signingconfig.v0.1+json or
// application/vnd.dev.sigstore.signingconfig.v0.2+json
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit unclear: after this PR this message documents v0.2 only but that's not said anywhere. Do we allow v0.1 to be used in future as well?

Bundle uses this language:

    // MUST be application/vnd.dev.sigstore.bundle.v0.3+json when encoded as JSON.
    // Clients must to be able to accept media type using the previously defined formats:
    // [list follows]

Of course situation here is a bit different: we should document

  • what we expect the instance to serve
    • I'd like to say "MUST serve v0.2" but obviously there's a migration period here
  • what we expect clients to handle
    • MUST handle v0.2, SHOULD handle v0.1

protos/sigstore_trustroot.proto
string media_type = 5;

// Deprecated: Use certificate_authority_urls
Copy link
Member

@jku jku Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Can you explain the way these deprecations are useful (compared to just removing the fields):

  • if a service does not want to support the new fields, I would expect them to just keep serving signingconfig v0.1?
  • if a client supports signingvonfig v0.2 then they need to support the new fields so won't need these

Maybe I'm missing a case?

Copy link
Member

Choose a reason for hiding this comment

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

is the idea that clients have an easier time supporting v0.2 if they can just update version number and keep doing what they are doing now?

@jku
Copy link
Member

jku commented Feb 18, 2025 •
edited
Loading

I guess one option to make the transition to new SigningConfig as easy as possible is to not replace the old one but just offer a new one:

  • a new TUF artifact signing_config2.json is added alongside the existing signing_config.json
  • the clients can implement support on their schedule: instead of parsing signing_config.json they start download/parse signing_config2.json
  • at some point in future when enough clients have transitioned, signing_config.json artifact is removed from TUF repository

This would be a way to avoid a flag day for SigningConfig

@jku
Copy link
Member

jku commented Feb 18, 2025

  • I kept the old url fields because I'm not sure if that will break sigstore-python, which supports ClientTrustConfig.

pretty sure sigstore-python will not parse the new data without code changes since the signingconfig version number changes

@jku
Copy link
Member

jku commented Feb 18, 2025

I guess one option to make the transition to new SigningConfig as easy as possible is to not replace the old one but just offer a new one:

I'll have a better look at this again this week as I feel there is something to this idea -- we may be mistakenly combining the two problem cases (backwards incompatible service migration and plain log instance rotation), the right solutions to each might actually be different

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jku jku jku left review comments

@cmurphy cmurphy cmurphy left review comments

@kommendorkapten kommendorkapten kommendorkapten approved these changes

@loosebazooka loosebazooka Awaiting requested review from loosebazooka

@woodruffw woodruffw Awaiting requested review from woodruffw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SigningConfig proto to have start dates?
4 participants
@haydentherapper @jku @cmurphy @kommendorkapten