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

feat: add subject URIs to index for x509 certificates #897

Merged
merged 4 commits into from
Jul 1, 2022

Conversation

asraa
Copy link
Contributor

@asraa asraa commented Jun 30, 2022

Signed-off-by: Asra Ali asraa@google.com

Summary

Adds URIs to x509 certificate "email addresses". This picks up on job workflow ref for Fulcio issued certificates.

Ideally I'd like to deprecate EmailAddresses() in favor of Subjects() -- should I add Subjects() and mark EmailAddresses() as deprecated? I'm not sure there's much external usage.

Ticket Link

Fixes

Release Note

* pki: Deprecates `pki.PublicKey.EmailAddresses()`. Use `pki.PublicKey.Subjects()` instead to include subject URIs from public keys like x509 certificates.

Signed-off-by: Asra Ali <asraa@google.com>
@asraa asraa requested a review from a team as a code owner June 30, 2022 14:03
Comment on lines 193 to 197
errs := validate.Var(name.String(), "required,uri")
if errs == nil {
names = append(names, strings.ToLower(name.String()))
}
}
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
errs := validate.Var(name.String(), "required,uri")
if errs == nil {
names = append(names, strings.ToLower(name.String()))
}
}
if errs := validate.Var(name.String(), "required,uri"); errs == nil {
names = append(names, strings.ToLower(name.String()))
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Are these validation errors surfaced anywhere before/after this? It seems a waste to drop perfectly good validation errors from end users (...says the end user most likely to send invalid values 🙃 )

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 ones a little tougher to surface -- I expect that this one is relatively difficult to hit, because Fulcio is the one populating certificates and it's not user-controlled.

On that note maybe it's worth filing an issue that we surface what keys WERE successfully indexed when you upload something?

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, it's also worth noting that it might not be Fulcio's certs uploaded to the log, as users can upload certs from their own PKI.

pkg/pki/x509/x509.go Outdated Show resolved Hide resolved
@bobcallaway
Copy link
Member

Ideally I'd like to deprecate EmailAddresses() in favor of Subjects() -- should I add Subjects() and mark EmailAddresses() as deprecated? I'm not sure there's much external usage.

I'm fine with that.

@imjasonh
Copy link
Member

Ideally I'd like to deprecate EmailAddresses() in favor of Subjects() -- should I add Subjects() and mark EmailAddresses() as deprecated? I'm not sure there's much external usage.

I looked, and sget uses EmailAddresses(), but I don't mind having to switch when I bump the dep.

Signed-off-by: Asra Ali <asraa@google.com>
@asraa
Copy link
Contributor Author

asraa commented Jun 30, 2022

Added Subjects() and a deprecation notice on EmailAddresses()

Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
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