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

x509-cert: rework certificate builder profiles #1306

Merged
merged 1 commit into from May 13, 2024
Merged

x509-cert: rework certificate builder profiles #1306

merged 1 commit into from May 13, 2024

Conversation

baloo
Copy link
Member

@baloo baloo commented Jan 5, 2024 •
edited

I tried to abide by the ETSI policy.
https://www.etsi.org/deliver/etsi_en/319400_319499/31941202/02.03.01_60/en_31941202v020301p.pdf

The policy recommends to make KeyUsage bits 0, 1, (2 and/or 4) exclusive. This is meant to avoid making signing of commitments during authentication.

This commit goes a bit further by making bit 2 and 4 exclusive. I don't find a use-case for requiring both at the same time (and I would think only key agreement is required.

Fixes #1281
Fixes #1392

@baloo baloo mentioned this pull request Jan 5, 2024
Closed
@baloo
Copy link
Member Author

baloo commented Jan 5, 2024

cc @lumag

@baloo baloo mentioned this pull request Jan 5, 2024
Open
@lumag
Copy link
Contributor

lumag commented Jan 5, 2024

I took a glance a the GOST certificate which is used in the signature on my Ham license. It has:

		Key Usage (critical):
			Digital signature.
			Non repudiation.
			Key encipherment.
			Data encipherment.

-----BEGIN CERTIFICATE-----
MIIIyjCCCHegAwIBAgIRAWOoLLPymOeA6RHkuLNA5wEwCgYIKoUDBwEBAwIwggER
MR8wHQYJKoZIhvcNAQkBFhBwa2ktZ3JmY0BncmZjLnJ1MRgwFgYFKoUDZAESDTEw
Mjc3MzkzMzQ0NzkxGjAYBggqhQMDgQMBARIMMDA3NzA2MjI4MjE4MQswCQYDVQQG
EwJSVTEcMBoGA1UECAwTNzcg0LMuINCc0L7RgdC60LLQsDEVMBMGA1UEBwwM0JzQ
vtGB0LrQstCwMTowOAYDVQQJDDHQlNC10YDQsdC10L3QtdCy0YHQutCw0Y8g0L3Q
sNCxLiDQtC4gNyDRgdGC0YAuIDE1MRwwGgYDVQQKDBPQpNCT0KPQnyAi0JPQoNCn
0KYiMRwwGgYDVQQDDBPQpNCT0KPQnyAi0JPQoNCn0KYiMB4XDTE5MDgwNzA3MTE0
NVoXDTIwMDgwNzA3MjE0NVowggIdMSIwIAYJKoZIhvcNAQkBFhNhLnN0cm9nYW5v
dkBncmZjLnJ1MRYwFAYFKoUDZAMSCzEwNjg4MjMzNDY1MRgwFgYFKoUDZAESDTEw
Mjc3MzkzMzQ0NzkxGjAYBggqhQMDgQMBARIMMDA3NzA2MjI4MjE4MTowOAYDVQQJ
DDHQlNC10YDQsdC10L3QtdCy0YHQutCw0Y8g0L3QsNCxLiDQtC4gNyDRgdGC0YAu
IDE1MRUwEwYDVQQHDAzQnNC+0YHQutCy0LAxHDAaBgNVBAgMEzc3INCzLiDQnNC+
0YHQutCy0LAxCzAJBgNVBAYTAlJVMS4wLAYDVQQqDCXQkNC70LXQutGB0LDQvdC0
0YAg0KHQtdGA0LPQtdC10LLQuNGHMRswGQYDVQQEDBLQodGC0YDQvtCz0LDQvdC+
0LIxNzA1BgNVBAwMLtCd0LDRh9Cw0LvRjNC90LjQuiDRg9C/0YDQsNCy0LvQtdC9
0LjRjyDQrdCg0KcxZTBjBgNVBAsMXNCj0L/RgNCw0LLQu9C10L3QuNC1INCy0LXR
idCw0YLQtdC70YzQvdGL0YUg0Lgg0LvRjtCx0LjRgtC10LvRjNGB0LrQvtC5INGA
0LDQtNC40L7RgdC70YPQttCxMR4wHAYDVQQKDBXQpNCT0KPQnyDCq9CT0KDQp9Cm
wrsxHjAcBgNVBAMMFdCk0JPQo9CfIMKr0JPQoNCn0KbCuzBmMB8GCCqFAwcBAQEB
MBMGByqFAwICJAAGCCqFAwcBAQICA0MABEBYpnf84L0TN1keZ1cHms/e86jD43Pe
JG+Qc5ujzroSmdDd9qYwFHLSwU063iuclD24Gugi0H+h2SCXC/RKoVyDo4IEkTCC
BI0wDgYDVR0PAQH/BAQDAgTwMB0GA1UdIAQWMBQwCAYGKoUDZHEBMAgGBiqFA2Rx
AjAsBgUqhQNkbwQjDCHQodCa0JfQmCAi0JrRgNC40L/RgtC+0J/RgNC+IENTUCIw
JwYDVR0lBCAwHgYIKwYBBQUHAwIGCCsGAQUFBwMEBggqhQMDBQoCDDCCAWAGA1Ud
IwSCAVcwggFTgBTZLA2zDzTrbwMD+B7BHiS2B8QNM6GCASykggEoMIIBJDEeMBwG
CSqGSIb3DQEJARYPZGl0QG1pbnN2eWF6LnJ1MQswCQYDVQQGEwJSVTEYMBYGA1UE
CAwPNzcg0JzQvtGB0LrQstCwMRkwFwYDVQQHDBDQsy4g0JzQvtGB0LrQstCwMS4w
LAYDVQQJDCXRg9C70LjRhtCwINCi0LLQtdGA0YHQutCw0Y8sINC00L7QvCA3MSww
KgYDVQQKDCPQnNC40L3QutC+0LzRgdCy0Y/Qt9GMINCg0L7RgdGB0LjQuDEYMBYG
BSqFA2QBEg0xMDQ3NzAyMDI2NzAxMRowGAYIKoUDA4EDAQESDDAwNzcxMDQ3NDM3
NTEsMCoGA1UEAwwj0JzQuNC90LrQvtC80YHQstGP0LfRjCDQoNC+0YHRgdC40LiC
CwCJt/KLAAAAAAGwMB0GA1UdDgQWBBTAqbdpE3IWcbpS/WezyoPc7CXETzArBgNV
HRAEJDAigA8yMDE5MDgwNzA3MTE0NFqBDzIwMjAwODA3MDcxMTQ0WjCCARwGBSqF
A2RwBIIBETCCAQ0MNNCh0JrQl9CYICLQmtGA0LjQv9GC0L7Qn9GA0L4gQ1NQIiAo
0LLQtdGA0YHQuNGPIDQuMCkMM9Cf0JDQmiAi0JrRgNC40L/RgtC+0J/RgNC+INCj
0KYiICjQstC10YDRgdC40LggMi4wKQxP0KHQtdGA0YLQuNGE0LjQutCw0YIg0YHQ
vtC+0YLQstC10YLRgdGC0LLQuNGPIOKEliDQodCkLzEyNC0yODY0INC+0YIgMjAu
MDMuMjAxNgxP0KHQtdGA0YLQuNGE0LjQutCw0YIg0YHQvtC+0YLQstC10YLRgdGC
0LLQuNGPIOKEliDQodCkLzEyOC0yOTgzINC+0YIgMTguMTEuMjAxNjCBoQYDVR0f
BIGZMIGWMEmgR6BFhkNodHRwOi8vcGtpLmdyZmMucnUvY2RwL2Q5MmMwZGIzMGYz
NGViNmYwMzAzZjgxZWMxMWUyNGI2MDdjNDBkMzMuY3JsMEmgR6BFhkNodHRwOi8v
cGtpLmdyZmMucnUvY2RwL2Q5MmMwZGIzMGYzNGViNmYwMzAzZjgxZWMxMWUyNGI2
MDdjNDBkMzMuY3JsMIGQBggrBgEFBQcBAQSBgzCBgDAtBggrBgEFBQcwAYYhaHR0
cDovL3BraS5ncmZjLnJ1L29jc3AyL29jc3Auc3JmME8GCCsGAQUFBzAChkNodHRw
Oi8vcGtpLmdyZmMucnUvYWlhL2Q5MmMwZGIzMGYzNGViNmYwMzAzZjgxZWMxMWUy
NGI2MDdjNDBkMzMuY3J0MAoGCCqFAwcBAQMCA0EAOZ0ADB7slKar4+TMOgM76VCH
tKKpsl/U3R05Nr9mSFB3mludmw6hyTUleOl6eWWtJlpqpHeKUgvPNpBL95RPLw==
-----END CERTIFICATE-----

@lumag
Copy link
Contributor

lumag commented Jan 5, 2024

Some justification. GOST is one of 'national' PK schemes. The corresponding X.509 parts are described in RFC 4491 and RFC 9215. Certificates can be used for both digital signatures, key agreement and key transport, see RFC 4490.

@carl-wallace
Copy link
Contributor

I don't think I've seen nonRepudiation and keyEncipherment together before as the latter often is accompanied by escrow of the private key. I've not caught up yet to current release to try it out but the PR looks OK.

@baloo
Copy link
Member Author

baloo commented Jan 5, 2024

We still allow the use to set nonRepudiation and keyEncipherment by relying on the x509_cert::builder::Profile::Manual this is behind the hazmat feature flag.

I think that's in line with the security note from ETSI:

NOTE 2: [security note] Combining the non-repudiation bit (bit 1) in the keyUsage certificate extension with other
keyUsage bits can have security implications depending on the security environment in which the
certificate is to be used.
If the subject's environment can be fully controlled and trusted, then there are no specific security
implications. For example, in cases where the subject is fully confident about exactly which data is signed
or cases where the subject is fully confident about the security characteristics of the authentication
protocol being used.
If the subject's environment is not fully controlled or not fully trusted, then unintentional signing of
commitments is possible. Examples include the use of badly formed authentication exchanges and the use
of a rogue software component.

@lumag
Copy link
Contributor

lumag commented Jan 5, 2024

That note talks about the nonRepudiation bit. However I'm more concerned about mixing of other usage bits.
For example RFC 3279 section 2.3.5 (also see errata 6672). It explicitly allows any combination of digitalSignature, nonRepudiation and keyAgreement. Even if we apply the mentioned note from ETSI standard, we should allow digitalSignature + keyAgreement.

This extends to GOST certs which can have digitalSignature + keyEncipherment + dataEncipherment.

@lumag
Copy link
Contributor

lumag commented Jan 5, 2024

Forr the reference, RFC 4055 allows keyEncipherment and dataEncipherment, but explicitly tellls that those two bits SHOULD NOT be used together.

@baloo
Copy link
Member Author

baloo commented Jan 6, 2024 •
edited

yeaaah, there is more to this story.
https://cabforum.org/wp-content/uploads/CA-Browser-Forum-BR-v2.0.1.pdf#page=80 (7.1.2.7.11 Subscriber Certificate Key Usage)

I'll revisit this.

There are also code signing certificates, with different requirements.

@baloo baloo marked this pull request as draft January 6, 2024 00:15
@baloo baloo force-pushed the baloo/x509-cert/builder-keyusage branch 2 times, most recently from b5dc812 to b2b6b6d Compare January 17, 2024 00:27
@baloo
Copy link
Member Author

baloo commented Jan 19, 2024 •
edited

@tarcieri can I ask for your opinion on this trait based approach?
I'm not done yet. I'd like to also provide a set of implementation for code-signing profiles and get a PIV profile as well.

@baloo baloo force-pushed the baloo/x509-cert/builder-keyusage branch 3 times, most recently from 9fa3709 to 7213a65 Compare January 21, 2024 06:39
@tarcieri
Copy link
Member

@baloo as a general direction it looks great!

@baloo baloo force-pushed the baloo/x509-cert/builder-keyusage branch from 7213a65 to 748dce8 Compare January 29, 2024 08:11
@baloo baloo mentioned this pull request Apr 30, 2024
Closed
@baloo baloo force-pushed the baloo/x509-cert/builder-keyusage branch 2 times, most recently from ad8b0d5 to 4daa75e Compare April 30, 2024 23:06
@baloo baloo marked this pull request as ready for review April 30, 2024 23:08
@baloo baloo marked this pull request as draft April 30, 2024 23:09
@baloo baloo force-pushed the baloo/x509-cert/builder-keyusage branch from 4daa75e to aad2c72 Compare April 30, 2024 23:23
@baloo baloo marked this pull request as ready for review April 30, 2024 23:24
baloo
baloo commented Apr 30, 2024
View reviewed changes
x509-cert/src/builder.rs
@@ -38,6 +37,23 @@ pub enum Error {

/// Signing error propagated for the [`signature::Error`] type.
Signature(signature::Error),

/// Each RelativeDistinguishedName MUST contain exactly one AttributeTypeAndValue.
NonUniqueRdn,
Copy link
Member Author

Choose a reason for hiding this comment

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

Those new error entries are mostly tied to the checks run for compliance with cabf, I could probably make a subtype?

@baloo baloo changed the title x509-cert: fix KeyUsage on leaf certificates builder x509-cert: rework certificate builder profiles May 7, 2024
@baloo baloo force-pushed the baloo/x509-cert/builder-keyusage branch from aad2c72 to fd4a90e Compare May 7, 2024 19:58
@baloo
x509-cert: rework the profile of builder
8e7d9c7 
This is now providing a trait to be implemented by the consumer.

A number of implementation are available, including ones trying to abide
by CABF Baseline Requirements.

Fixes RustCrypto#1281
@baloo baloo force-pushed the baloo/x509-cert/builder-keyusage branch from fd4a90e to 8e7d9c7 Compare May 7, 2024 20:08
@baloo
Copy link
Member Author

baloo commented May 12, 2024

I intend to merge this tomorrow, unless someone has something to say about it.

@baloo baloo merged commit d1cac63 into RustCrypto:master May 13, 2024
53 checks passed
@baloo baloo deleted the baloo/x509-cert/builder-keyusage branch May 13, 2024 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tarcieri tarcieri tarcieri approved these changes

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

Successfully merging this pull request may close these issues.

x509-cert: local zlint failures x509-cert: Non-Repudiation set by default in builder
4 participants
@baloo @lumag @carl-wallace @tarcieri