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

[API Proposal]: Additional Hash Algorithms for X509SubjectKeyIdentifierHashAlgorithm #97158

Closed
vcsjones opened this issue Jan 18, 2024 · 9 comments · Fixed by #98579
Closed
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Security in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@vcsjones
Copy link
Member

vcsjones commented Jan 18, 2024

Background and motivation

In PKIX / X.509, the SubjectKeyIdentifier and AuthorityKeyIdentifier are opaque identifiers, however traditionally they have been derived from a SHA-1 over the subjectPublicKey.

SHA-1 has largely been discouraged for a long time. Even in places that are not strictly security, such as SKI and AKI, the use of SHA-1 comes with scrutiny from a compliance perspective, and requires an ongoing "exception" process.

Today, we only support a few flavors of SHA-1 with X509SubjectKeyIdentifierHashAlgorithm. This proposal is to add other hash algorithms as defined by RFC 7093.

API Proposal

namespace System.Security.Cryptography.X509Certificates;

public enum X509SubjectKeyIdentifierHashAlgorithm {
    Sha1 = 0,
    ShortSha1 = 1,
    CapiSha1 = 2,
+   Rfc7093TruncatedSha256 = 3, //  leftmost 160-bits of the SHA-256 hash over subjectPublicKey
+   Rfc7093TruncatedSha384 = 4, //  leftmost 160-bits of the SHA-384 hash over subjectPublicKey
+   Rfc7093TruncatedSha512 = 5, //  leftmost 160-bits of the SHA-512 hash over subjectPublicKey
+   Rfc7093Sha256 = 6, // Full SHA-256 hash over SubjectPublicKeyInfo
+   Rfc7093Sha384 = 7, // Full SHA-384 hash over SubjectPublicKeyInfo
+   Rfc7093Sha512 = 8, // Full SHA-512 hash over SubjectPublicKeyInfo
}

API Usage

X509SubjectKeyIdentifierExtension mySki = new(
    myPublicKey,
    X509SubjectKeyIdentifierHashAlgorithm. Rfc7093TruncatedSha256,
    critical: false);

Alternative Designs

No response

Risks

No response

@vcsjones vcsjones added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 18, 2024
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 18, 2024
@ghost
Copy link

ghost commented Jan 18, 2024

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

In PKIX / X.509, the SubjectKeyIdentifier and AuthorityKeyIdentifier are opaque identifiers, however traditionally they have been derived from a SHA-1 over the subjectPublicKey.

SHA-1 has largely been discouraged for a long time. Even in places that are not strictly security, such as SKI and AKI, the use of SHA-1 comes with scrutiny from a compliance perspective.

Today, we only support a few flavors of SHA-1 with X509SubjectKeyIdentifierHashAlgorithm. This proposal is to add other hash algorithms as defined by RFC 7093.

API Proposal

namespace System.Security.Cryptography.X509Certificates;

public enum X509SubjectKeyIdentifierHashAlgorithm {
    Sha1 = 0,
    ShortSha1 = 1,
    CapiSha1 = 2,
+   TruncatedSha256 = 3, //  leftmost 160-bits of the SHA-256 hash
+   TruncatedSha384 = 4, //  leftmost 160-bits of the SHA-384 hash
+   TruncatedSha512 = 5, //  leftmost 160-bits of the SHA-512 hash
+   Sha256 = 6,
+   Sha384 = 7,
+   Sha512 = 8,
}

API Usage

X509SubjectKeyIdentifierExtension mySki = new(
    myPublicKey,
    X509SubjectKeyIdentifierHashAlgorithm.TruncatedSha256,
    critical: false);

Alternative Designs

No response

Risks

No response

Author: vcsjones
Assignees: -
Labels:

api-suggestion, area-System.Security

Milestone: -

@bartonjs
Copy link
Member

"Why does the new proposal have 'Truncated' entries when the existing one has 'Short'?"

RFC 5280, 4.2.1.2 defines the second method of computation as the 64-bit value 0b0100 concat the 60 rightmost bits of the SHA-1; but RFC 7039, 2 defines the new ones as the leftmost 160 bits of the bigger algorithms. It's a different kind of short, so a different word.

"Oh, OK, fair enough."

@bartonjs
Copy link
Member

And the non-truncated ones moved from just the encoded key to the full SPKI? Ugh.

Then maybe I do want to re-assert my "withheld" Short vs Truncated and say that the API doesn't need to really show these differences, they can be covered in docs.

public enum X509SubjectKeyIdentifierHashAlgorithm {
    Sha1 = 0, // SHA-1 over the subjectPublicKey
    ShortSha1 = 1, // 0b0100 concat rightmost 60 bits of the SHA-1 over subjectPublicKey
    CapiSha1 = 2, // Some CAPI thing
+   ShortSha256 = 3, //  leftmost 160-bits of the SHA-256 hash over subjectPublicKey
+   ShortSha384 = 4, //  leftmost 160-bits of the SHA-384 hash over subjectPublicKey
+   ShortSha512 = 5, //  leftmost 160-bits of the SHA-512 hash over subjectPublicKey
+   Sha256 = 6, // SHA-256 hash over the full SubjectPublicKeyInfo
+   Sha384 = 7, // SHA-384 hash over the full SubjectPublicKeyInfo
+   Sha512 = 8, // SHA-512 hash over the full SubjectPublicKeyInfo
}

That looks a bit cleaner to me.

@vcsjones
Copy link
Member Author

And the non-truncated ones moved from just the encoded key to the full SPKI? Ugh.

Yeah, but I did want to convey they are different, hence Truncated and SpkiFull. Which, yeah, are a little convoluted.

It seems equally "weird" to me that Sha1 and Sha256 are hashes over different data, let alone different algorithms. It's probably difficult to exactly convey everything in the enum name, but I was aiming for some distinction in the name, if anything so that it would steer people toward reading the docs rather than assume it is a simple algorithm change.

@bartonjs
Copy link
Member

Well, we can fall back on good old "blame the RFC", and call it Rfc7093Sha256 or Sha256Rfc7093. I looked up what the "CAPI" variant does, and we'd be "reasonable" calling it CapiSha256, though that implies (to me) "this is a mistake, don't use it", so probably not that one.

@vcsjones
Copy link
Member Author

and call it Rfc7093Sha256 or Sha256Rfc7093.

Well, there are two SHA-256s. One truncated over subjectPublicKey and another full over SPKI. So Rfc7093Sha256 and Rfc7093TruncatedSha256?

and we'd be "reasonable" calling it CapiSha256

Uh...

"this is a mistake, don't use it"

Yes that.

@vcsjones
Copy link
Member Author

Okay, I optimistically took another guess at naming.

@bartonjs
Copy link
Member

Don't love it, don't hate it... but we can see what the group thinks.

@bartonjs bartonjs added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 19, 2024
@bartonjs bartonjs added this to the 9.0.0 milestone Jan 19, 2024
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jan 19, 2024
@bartonjs
Copy link
Member

bartonjs commented Feb 13, 2024

Video

  • We dropped the RFC7093 prefix, that part can be explained in docs.
  • We went with "Short" instead of "Truncated". Even though it's a different style of truncation algorithm it's not important to distinguish in the API.
  • @terrajobst was bothered by the old values being "(Full)" followed by "Short", so we flipped 3-5 and 6-8.
public enum X509SubjectKeyIdentifierHashAlgorithm {
    Sha1 = 0, // SHA-1 over the subjectPublicKey
    ShortSha1 = 1, // 0b0100 concat rightmost 60 bits of the SHA-1 over subjectPublicKey
    CapiSha1 = 2, // Some CAPI thing
+   Sha256 = 3, // SHA-256 hash over the full SubjectPublicKeyInfo
+   Sha384 = 4, // SHA-384 hash over the full SubjectPublicKeyInfo
+   Sha512 = 5, // SHA-512 hash over the full SubjectPublicKeyInfo
+   ShortSha256 = 6, //  leftmost 160-bits of the SHA-256 hash over subjectPublicKey
+   ShortSha384 = 7, //  leftmost 160-bits of the SHA-384 hash over subjectPublicKey
+   ShortSha512 = 8, //  leftmost 160-bits of the SHA-512 hash over subjectPublicKey
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 13, 2024
@vcsjones vcsjones self-assigned this Feb 13, 2024
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 16, 2024
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 17, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 18, 2024
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Aug 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Security in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants