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

TLS Cipher Suite 0xC102 Support #11403

Closed
wants to merge 1 commit into from
Closed

Conversation

NMorozxov
Copy link
Contributor

@NMorozxov NMorozxov commented Mar 25, 2020

For GOST2012-GOST8912-GOST8912 was used 0xFF85 identifier,
but new identifier 0xc102 was assigned.
Because of old software we will support both numbers.

https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-2
https://datatracker.ietf.org/doc/draft-smyshlyaev-tls12-gost-suites/

Checklist
  • documentation is added or updated
  • tests are added or updated

@kaduk
Copy link
Contributor

kaduk commented Mar 26, 2020

I am wondering whether there would be a desire for a knob to provide "strictly compliant" behavior (i.e., only recognize 0xc102 and not 0xff85), and what such a knob would look like.
We usually do not have knobs for such narrow behavior changes, though, so it seems somewhat unlikely that we would add such a thing.

@mattcaswell
Copy link
Member

This change will cause backwards compat problems. Because we rename the old cipher to LEGACY-GOST2012-GOST8912-GOST8912 this means that any existing configs will automatically start using the new definition and cipher id for GOST2012-GOST8912-GOST8912. This will fail to interoperate with any existing installations that don't yet understand the new cipher-id. This will force all users to amend their configs to list both the old and the new values.

One possibility is to arrange things so that if clients have GOST2012-GOST8912-GOST8912 configured then they always send both cipher ids in the ClientHello (with the new one before the old one). On the server side, if GOST2012-GOST8912-GOST8912 is configured then it could accept either cipher id.

@NMorozxov
Copy link
Contributor Author

How i can fo this arrangement?

  1. i can combine LEGACY-GOST2012-GOST8912-GOST8912 and NEW-GOST2012-GOST8912-GOST8912 to GOST2012-GOST8912-GOST8912 to a group, but there is no group expext STRONG, may be i must add a new one
  2. I can fix code in place where the ciphers is sended

@NMorozxov
Copy link
Contributor Author

Or i can leave the old one, and add a new one IANA-GOST2012-GOST8912-GOST8912 or something like this

@mattcaswell
Copy link
Member

i can combine LEGACY-GOST2012-GOST8912-GOST8912 and NEW-GOST2012-GOST8912-GOST8912 to GOST2012-GOST8912-GOST8912 to a group, but there is no group expext STRONG, may be i must add a new one

This sounds plausible - but perhaps do it as an alias. See cipher_aliases in ssl_ciph.c

@beldmit
Copy link
Member

beldmit commented Mar 31, 2020

@NMorozxov, do you have plans to implement Matt's suggestion?

@NMorozxov
Copy link
Contributor Author

@beldmit I have pushed yesterday. Two chippers and alias. What suggestions did i miss ?

@beldmit
Copy link
Member

beldmit commented Mar 31, 2020

Sorry, I missed the commit. Thanks!

ssl/t1_trce.c Outdated Show resolved Hide resolved
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

Looks good, but probably there also needs to be some updates in doc/man1/ciphers.pod

For GOST2012-GOST8912-GOST8912 was used 0xFF85 identifier,
but new identifier 0xc102 was assigned.
Because of old software we will support both numbers.

https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-2
https://datatracker.ietf.org/doc/draft-smyshlyaev-tls12-gost-suites/
@NMorozxov
Copy link
Contributor Author

@mattcaswell @beldmit fix submited

@beldmit
Copy link
Member

beldmit commented Apr 1, 2020

Approve if Travis is happy

@mattcaswell mattcaswell added the approval: done This pull request has the required number of approvals label Apr 1, 2020
@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but this PR has failing CI tests. Once the tests pass it will get moved to 'approval: ready to merge' automatically, alternatively please review and set the label manually.

openssl-machine pushed a commit that referenced this pull request Apr 2, 2020
For GOST2012-GOST8912-GOST8912 was used 0xFF85 identifier,
but new identifier 0xc102 was assigned.
Because of old software we will support both numbers.

https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-2
https://datatracker.ietf.org/doc/draft-smyshlyaev-tls12-gost-suites/

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from #11403)
@beldmit
Copy link
Member

beldmit commented Apr 2, 2020

Merged. Many thanks!

@beldmit beldmit closed this Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants