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

Change username format, enforce identity format #802

Merged
merged 5 commits into from Sep 28, 2022

Conversation

haydentherapper
Copy link
Contributor

@haydentherapper haydentherapper commented Sep 26, 2022

Summary

This updates the username type to avoid the username subject format
looking like an email. Fulcio will now specify the subject in the
OtherName SAN, and the format will use a ! instead of @.

This required some custom ASN.1 marshalling and unmarshalling, since
crypto/x509 does not support the OtherName SAN.

This also adds enforcement that email subjects match a basic email
regex format, and that other types do not look like emails.

Fixes #716

Release Note

Changed username identity format to username!Domain, username now specified in the OtherName SAN. If you have deployed your own instance of Fulcio and are using username issuers, you must update to the latest Cosign release.

Documentation

This updates the username type to avoid the username subject format
looking like an email. Fulcio will now specify the subject in the
OtherName SAN, and the format will use a ! instead of @.

This required some custom ASN.1 marshalling and unmarshalling, since
crypto/x509 does not support the OtherName SAN.

This also adds enforcement that email subjects match a basic email
regex format, and that other types do not look like emails.

Fixes sigstore#716

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@haydentherapper
Copy link
Contributor Author

cc @woodruffw - Thanks for the suggestion to use OtherName! You also gave me an excuse to dig a bit more into ASN.1 encoding, which may or may not have been productive. :)

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
pkg/identity/email/principal.go Outdated Show resolved Hide resolved
pkg/identity/uri/principal.go Outdated Show resolved Hide resolved
pkg/identity/username/principal.go Outdated Show resolved Hide resolved
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@codecov-commenter
Copy link

Codecov Report

Merging #802 (9eeea6f) into main (f3dd6a9) will increase coverage by 1.01%.
The diff coverage is 86.20%.

@@            Coverage Diff             @@
##             main     #802      +/-   ##
==========================================
+ Coverage   54.54%   55.56%   +1.01%     
==========================================
  Files          36       37       +1     
  Lines        2275     2354      +79     
==========================================
+ Hits         1241     1308      +67     
- Misses        942      950       +8     
- Partials       92       96       +4     
Impacted Files Coverage Δ
pkg/certificate/extensions.go 100.00% <ø> (ø)
pkg/identity/username/principal.go 92.68% <84.21%> (-7.32%) ⬇️
pkg/identity/username/othername.go 85.48% <85.48%> (ø)
pkg/identity/email/principal.go 91.66% <100.00%> (+0.75%) ⬆️
pkg/identity/uri/principal.go 93.18% <100.00%> (+0.49%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@woodruffw
Copy link
Member

Exciting! I'll update the relevant bits on sigstore-python once this lands 🙂

@haydentherapper haydentherapper changed the title Change username format, enforce identity format DNS: Change username format, enforce identity format Sep 27, 2022
@haydentherapper
Copy link
Contributor Author

Found issue, crypto/x509 doesn't handle a critical SAN extension with a GeneralName like OtherName (not DNS/IP/email/URI). Either needs to be non-critical or figure out if we can fix this in the library

@haydentherapper
Copy link
Contributor Author

AI: Look at certificate-transparency-go

@haydentherapper haydentherapper changed the title DNS: Change username format, enforce identity format Change username format, enforce identity format Sep 28, 2022
@haydentherapper
Copy link
Contributor Author

Found a way to handle this! You can remove unhandled critical extensions before verifying, there's a cert.UnhandledCriticalExtensions array of OIDs populated by x509.ParseCertificate. Just remove the SAN OID before verifying and we're good to go. This should be ready to merge now, and I'll get the Cosign update in.

I've filed a proposal in Go to add support for OtherName too.

@haydentherapper haydentherapper merged commit 928d977 into sigstore:main Sep 28, 2022
@haydentherapper haydentherapper deleted the extra-checks-fix-un branch September 28, 2022 16:10
@znewman01
Copy link
Contributor

@haydentherapper just one comment: a "username" would cause an existing Cosign client to choke, but you found an easy fix and we can roll that out to clients soon. In the meantime, we don't actually have username issuers configured in the public instance so it's not a problem in practice, and we just need to wait. Is that right?

The other fixes (e.g., verifying email format) won't break anybody because we've haven't seen any SANs that don't match so we don't need to worry there.

@haydentherapper
Copy link
Contributor Author

Yes, correct. No impact to production because we don't have any username issuers currently. This could impact someone who's deployed their own instance of Fulcio, but I am doubtful anyone is using the username identity right now, since it's not thoroughly documented. Also it would only impact them once they've updated to the latest Fulcio, so I'll make a note in the release notes to update to the latest Cosign, and we'll not cut a new release until the Cosign fix is out.

@hslatman
Copy link

@haydentherapper do you have an example certificate that includes the OtherName you can share? I'm adding support for printing the Sigstore OIDs to smallstep/certinfo in this PR: smallstep/certinfo#18 and would like to test the output of this new OID too.

@haydentherapper
Copy link
Contributor Author

Sweet, awesome to see this being added! This cert was generated from the tests:

-----BEGIN CERTIFICATE-----
MIICtDCCAlqgAwIBAgIUT3Bh3mWmJp+2wTQUxj19R1vYVKcwCgYIKoZIzj0EAwIw
aDEMMAoGA1UEBhMDVVNBMQswCQYDVQQIEwJXQTERMA8GA1UEBxMIS2lya2xhbmQx
FTATBgNVBAkTDDc2NyA2dGggU3QgUzEOMAwGA1UEERMFOTgwMzMxETAPBgNVBAoT
CHNpZ3N0b3JlMB4XDTIyMDkyOTE4MzcyMVoXDTIyMDkyOTE4NDcyMVowADBZMBMG
ByqGSM49AgEGCCqGSM49AwEHA0IABJUlrKgma8vvHGsin0Lns8ilkZIGfbFoNhp/
e2HPdLPwmy1IfigG2FRmMjw7DymnC3ce9o1k0zkcCuhc0Pn6PnOjggFIMIIBRDAO
BgNVHQ8BAf8EBAMCB4AwEwYDVR0lBAwwCgYIKwYBBQUHAwMwHQYDVR0OBBYEFDst
jNbwcRDr33q5Tt0kUjPLrG0sMB8GA1UdIwQYMBaAFN8zJRpf/idR3ooX82oyJ5BC
UFGjMCsGA1UdEQEB/wQhMB+gHQYKKwYBBAGDvzABB6APDA1mb28hMTI3LjAuMC4x
MCQGCisGAQQBg78wAQEEFmh0dHA6Ly8xMjcuMC4wLjE6NDU2NjEwgYkGCisGAQQB
1nkCBAIEewR5AHcAdQAodhoYkCf77zzQ1hoBjXawUFcpx6dBG8y99gT0XUJhUwAA
AAAAAAU5AAAEAwBGMEQCIAhzbUnllt0pkfDnAvEI/4yEESxXn6d+8bJqQ4i/oU68
AiBZgxlvo/D//+6I0ztBODY5/U6qYrUZt4a4SX1Uk3y/xzAKBggqhkjOPQQDAgNI
ADBFAiEAogo/upgvfrjroS72BcIrPznne7NC6mOt0jDVFDfFgswCIArUlhr07ftG
KCl3yVaw8c5Ck5p5eRSNY/RqZ0YRC7Ir
-----END CERTIFICATE-----

@hslatman
Copy link

It works, @haydentherapper!

   ...
            X509v3 Authority Key Identifier:
                keyid:DF:33:25:1A:5F:FE:27:51:DE:8A:17:F3:6A:32:27:90:42:50:51:A3
            X509v3 Subject Alternative Name: critical
                Fulcio Identity: foo!127.0.0.1
            Fulcio OIDC Issuer:
                http://127.0.0.1:45661
            RFC6962 Certificate Transparency SCT:
   ...

What do you think would be the best name for these? I've used Fulcio now, because that's the direct parent OID for all, but Sigstore is probably better known and may need to be given some visibility in the output?

@haydentherapper
Copy link
Contributor Author

I think Sigstore. Fulcio is more or less an implementation detail of Sigstore, and we could imagine another identity service that supports issuing certificates with the same format.

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.

Enforce that identities are of an expected format
6 participants