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

Align SignatureScheme ALL-CAPS-VERBS with RESERVED labels. #980

Closed
wants to merge 1 commit into from

Conversation

davidben
Copy link
Contributor

Values in RESERVED labels, per the note at the top of Appendix B, MUST
NOT be sent. This conflicts other text which tags ecdsa_sha1 and
dsa_sha1 as SHOULD NOT.

Back in early drafts, {*, dsa} and {sha1, ecdsa} were not tagged
RESERVED and were merely SHOULD NOT in the text:
https://tools.ietf.org/html/draft-ietf-tls-tls13-11#section-6.3.2.1

Then things were redone as SignatureScheme with the intent of preserving
SHOULD NOTs and MUST NOTs. Accordingly, dsa_* values were defined, and
with SHOULD NOTs in prose.
#404

That was followed up by a cleanup change which left dsa_* values in
there, but not defined. Intentionally or not, this took away the SHOULD
NOT and left it with something unclear.
bed7281

Then the RESERVED tag was added, in response to the cleanup.
Intentionally or not, this kicked in the Appendix B MUST NOT, which
means TLS 1.3 implementations are forbidden from offering DSA to TLS 1.2
servers. Nonetheless, the SHOULD NOT reference to the now non-existent
and verboten dsa_sha1 remained.
#434

Next, an oversight in PR #404 was "corrected". PR #404 was intended to
leave SHOULD NOTs and MUST NOTs as-is but downgraded {sha1, ecdsa} to a
MUST NOT by omission. However, I did not notice the Appendix B text, so
my correction was, in fact, a no-op.
#488

Restoring ecdsa_sha1 was motivated by existing many implementations
still offering {sha1, ecdsa} at TLS 1.2, so it was not clear whether
removing it was realistic yet. (Notably, dependence on {sha1, rsa} aka
rsa_pkcs1_sha1 is known to be prevalent.) Since then, BoringSSL has
removed ecdsa_sha1, so that is some evidence it is unnecessary. NSS
still offers it, however.

So now we have a small mess on our hands. This PR attempts to bring
things to a self-consistent picture. Implementations I'm involved with
no longer offer ecdsa_sha1 or dsa_*, so I am personally fine with any
self-consistent option. For this PR, I went with:

Since PR#488 was accepted and even called out in the changelog, my
interpretation was that it should end at SHOULD NOT. That I failed to
actually implement originally is a bug.

DSA is less clear, but since there were two changes by two separate
people who chipped away at the SHOULD NOT, my interpretation is to leave
it at MUST NOT. I have taken the two changes to their logical
conclusion, removing the named dsa_*_RESERVED values and references to
non-existent dsa_sha1.

Values in RESERVED labels, per the note at the top of Appendix B, MUST
NOT be sent. This conflicts other text which tags ecdsa_sha1 and
dsa_sha1 as SHOULD NOT.

Back in early drafts, {*, dsa} and {sha1, ecdsa} were not tagged
RESERVED and were merely SHOULD NOT in the text:
https://tools.ietf.org/html/draft-ietf-tls-tls13-11#section-6.3.2.1

Then things were redone as SignatureScheme with the intent of preserving
SHOULD NOTs and MUST NOTs. Accordingly, dsa_* values were defined, and
with SHOULD NOTs in prose.
tlswg#404

That was followed up by a cleanup change which left dsa_* values in
there, but not defined. Intentionally or not, this took away the SHOULD
NOT and left it with something unclear.
tlswg@bed7281

Then the RESERVED tag was added, in response to the cleanup.
Intentionally or not, this kicked in the Appendix B MUST NOT, which
means TLS 1.3 implementations are forbidden from offering DSA to TLS 1.2
servers. Nonetheless, the SHOULD NOT reference to the now non-existent
and verboten dsa_sha1 remained.
tlswg#434

Next, an oversight in PR tlswg#404 was "corrected". PR tlswg#404 was intended to
leave SHOULD NOTs and MUST NOTs as-is but downgraded {sha1, ecdsa} to a
MUST NOT by omission. However, I did not notice the Appendix B text, so
my correction was, in fact, a no-op.
tlswg#488

Restoring ecdsa_sha1 was motivated by existing many implementations
still offering {sha1, ecdsa} at TLS 1.2, so it was not clear whether
removing it was realistic yet. (Notably, dependence on {sha1, rsa} aka
rsa_pkcs1_sha1 is known to be prevalent.) Since then, BoringSSL has
removed ecdsa_sha1, so that is some evidence it is unnecessary.  NSS
still offers it, however.

So now we have a small mess on our hands. This PR attempts to bring
things to a self-consistent picture. Implementations I'm involved with
no longer offer ecdsa_sha1 or dsa_*, so I am personally fine with any
self-consistent option. For this PR, I went with:

Since PR#488 was accepted and even called out in the changelog, my
interpretation was that it should end at SHOULD NOT. That I failed to
actually implement originally is a bug.

DSA is less clear, but since there were two changes by two separate
people who chipped away at the SHOULD NOT, my interpretation is to leave
it at MUST NOT. I have taken the two changes to their logical
conclusion, removing the named dsa_*_RESERVED values and references to
non-existent dsa_sha1.
Copy link
Contributor

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

BTW, part of the reason Firefox has trouble here is that what NSS advertises here also determines what we support if we negotiate TLS 1.0, and we don't have good enough telemetry on that. Mostly that's because I'm too lazy to do the legwork.

@@ -2244,16 +2244,18 @@ SignatureSchemeList value:
ed25519(0x0807),
ed448(0x0808),

/* Legacy algorithms */
ecdsa_sha1(0x0203),
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you want to move rsa_pkcs1_sha1 here? Doesn't it fall under the same basic rules? I mean, some of us have cut one of the two out, but the same logic applies to both. You could rationalize that by merging the description of the two into the last bullet point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it probably makes sense to call rsa_pkcs1_sha1 a legacy algorithm, even though it's sufficiently entrenched that we will still be using it for the near future.

@@ -2301,9 +2303,14 @@ EdDSA algorithms
: Indicates a signature algorithm using EdDSA as defined in
{{RFC8032}} or its successors. Note that these correspond to the
"PureEdDSA" algorithms and not the "prehash" variants.

ecdsa_sha1
Copy link
Contributor

Choose a reason for hiding this comment

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

List structure here is odd: you might want to label this as "Legacy Algorithms" and merge the paragraph that follows the list.

I believe that both follow the rule regarding certificates as well, but this implies that pkcs1 is exempt from the prohibition regarding use in TLS-itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to MT's comment here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Martin's first point, but would probably benefit from a bit more context on the second point.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is both ecdsa_sha1 and rsa_pkcs1_sha1 have the same property regarding their use. We don't like them, but we have to concede that they might appear in certificates.

@davegarrett
Copy link
Contributor

A simple compromise to make the situation with these codepoints very clear is to just make up another annotation, like "TRANSITIONAL" or something, and tack that onto the end of all of the "SHOULD NOT" ones vs. the current "RESERVED" for "MUST NOT" ones.

obsolete_RESERVED(0x0000..0x0200),
obsolete_RESERVED(0x0202),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really want to strip the labels off of these; they have prior meaning, and there's no reason to obfuscate it here. (they're already hidden up top and only in the appendix) What's the reason for this proposed change?

@ekr
Copy link
Contributor

ekr commented Apr 27, 2017

Updated with PR#990

@ekr ekr closed this Apr 27, 2017
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.

None yet

5 participants