search

openssl / technical-policies

Mirror of the repository for technical policies, governed by the OTC (OpenSSL Technical Committee)
22 stars 34 forks source link

Vote: Accept PR #19681 in 3.1 subject to normal review process #59

Closed romen closed 1 year ago

romen commented 1 year ago 
Topic: Accept PR #19681 in 3.1 subject to normal review process
Comment: Fundamentally OTC must decide if in the 3.1 release we should
         honor OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT as set (and
         default to UNCOMPRESSED) in our provider implementations, and
         apply corresponding changes for handling legacy keys.
Proposed by: Nicola Tuveri
Issue link: https://github.com/openssl/technical-policies/issues/59
Public: yes
Opened: 2022-11-15
Closed: 2022-11-27
Accepted:  yes  (for: 7, against: 0, abstained: 2, not voted: 2)

  Dmitry     [+1]
  Matt       [+0]
  Pauli      [ 0]
  Tim        [+1]
  Hugo       [+1]
  Richard    [+1]
  Shane      [+1]
  Tomas      [+1]
  Kurt       [  ]
  Matthias   [  ]
  Nicola     [+1]

Convenience link to the PR in question: https://github.com/openssl/openssl/pull/19681

romen commented 1 year ago

During the discussion in the OTC meeting today, we said:

  • We would be entitled to change this because the documentation does not specify we return a compressed key
  • However, it may break existing systems in practice which are parsing this information and only implemented the compressed format
  • Should we change this?

My remark on this is that by design we did not specify UNCOMPRESSED, COMPRESSED or HYBRID as the point conversion format to use, and intentionally left it open to provider implementations to decide which one to use and support by default.

If there are applications out there that are hardcoding behavior based on the fact that our 3.0 providers unconditionally used COMPRESSED, they are already non-compliant with our documentation, and potentially incompatible with 3rd party providers (and of course with future versions of OpenSSL providers).

romen commented 1 year ago

Also note that, intentionally, this vote does not address the related issue of potential regressions introduced in 3.0 compared to 1.1.1 when generating/(de)serializing EC keys. That investigation is being separately carried out in openssl/openssl#18320 and openssl/openssl#19365, and will lead to a separate discussion and decision on how to address any such regression in 3.0, 3.1, and master.

mattcaswell commented 1 year ago

IMO this ultimately comes down to whether we consider this a bug or not. If its not a bug then the decision as to whether to change this or not should really be an OMC one (since we're not supposed to change behaviour in minor releases except to fix a bug).

I actually do consider this a bug since the point conversion format is not being respected. But a potentially destabilising one to fix. It's unclear to me how destabilising.

Vote: [+0]

kroeckx commented 1 year ago

If we do consider this a bug, I expect 3.0 to also get fixed.

romen commented 1 year ago

If we do consider this a bug, I expect 3.0 to also get fixed.

I do consider this a bug that should be fixed in 3.0 as well, but from the discussion earlier today I got the impression we wanted to see how we feel about the risks associated with this change for 3.1, before deciding if this should be fixed also in 3.0.

paulidale commented 1 year ago

I really don't like the wording. If it is a bug it should go into 3.0 and 3.1. If it isn't a bug, we need an exception to put it into 3.1.

This is worded very much like it is the latter which IMO suggests excluding the former.

romen commented 1 year ago

I really don't like the wording. If it is a bug it should go into 3.0 and 3.1. If it isn't a bug, we need an exception to put it into 3.1.

This is worded very much like it is the latter which IMO suggests excluding the former.

It was my understanding from the conversation on the last OTC meeting that we had consensus on this being a bug. I was asked to open this vote because of the risks implied by this behavioral change, despite our documentation intentionally gives us room for this change, and applications should not depend on any single point compression format if they want to be backward and forward compatible with our providers and 3rd party providers. For this we wanted to separately handle if this should be applied to 3.1 first, and defer the discussion for 3.0 to after the outcome of this vote.

@paulidale Did I miss someone objecting to openssl/openssl#16595 being considered a bug?

paulidale commented 1 year ago

What I'm not wanting to see is a fix in 3.1 but not in 3.0. That helps nobody. I don't really mind how it's classified, so long as that is avoided.

mattcaswell commented 1 year ago

If we do consider this a bug, I expect 3.0 to also get fixed.

If it is a bug it should go into 3.0 and 3.1.

The determination of whether something is a bug or not affects whether or not it is eligible to be fixed in a minor release (3.1) or backported to a stable release (3.0). However we have had instances in the past where we have "fixed" something in a stable release that we considered a bug - but have then had to rapidly "unfix" it again because, although it is a bug, people were relying on the buggy behaviour and fixing it broke them. So just because something is a bug and is eligible to be backported to a stable release does not necessarily imply that it should be. So I think it is perfectly ok for us to say "it's a bug but we're only going to fix it in 3.1 not 3.0". What I don't think we can do is say "It's not a bug, but we're going to change it in 3.1 anyway". That would be an OMC decision IMO.

romen commented 1 year ago

Do we have room for amendments in the text once the vote has been opened? Should I cancel the vote and open a new one instead?

My amendment, to address the feedback so far received would be:

Topic: Accept PR #19681 as a bug fix in 3.1 subject to normal review process

I do think we already reached consensus during the OTC meetings (on Nov 15th, and already over 1 year ago when it was first triaged) that openssl/openssl#16595 was a bug, hence why I did not include it in the current version of the vote topic.

From @paulidale's feedback I gather that not including "as a bug fix" suggests this vote is an exception. But I disagree, the vote text does not explicitly request an exception to a policy, nor this is the first instance where a bug fix PR was subject to a vote when OTC felt there are risks in the behavioral change that fixes the bug.

I admit I still don't see a need to alter it, but if already Matt and Pauli are unhappy with the vote text, I welcome suggestions on how to proceed.

mattcaswell commented 1 year ago

I admit I still don't see a need to alter it, but if already Matt and Pauli are unhappy with the vote text,

I did not mean to imply I was unhappy with the vote text. I was merely elaborating on my thought processes on how I should vote.

t8m commented 1 year ago

I disagree with pauli that fixing this on 3.1 and above is helping nobody. No, being able to specify the format by means of setting the OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT is something that people can wait for in 3.1 release.

And yes, my opinion is, this is a bug fix because we specified something in docs (although perhaps not as cleanly as possible) and we did not implement it like that. On the other hand it has some slight potential for breaking existing uses which stable release updates should avoid as much as possible so I do not think this bug should be fixed on 3.0.

t8m commented 1 year ago

Vote: [+1]

slontis commented 1 year ago

Vote: [+1]

paulidale commented 1 year ago

Vote: [0]

slontis commented 1 year ago

Note that a pyca test is failing currently with this PR. This could potentially change my vote to -1.

t-j-h commented 1 year ago

Vote: [+1]

levitte commented 1 year ago

Vote: [+1]

t-j-h commented 1 year ago

Note that a pyca test is failing currently with this PR. This could potentially change my vote to -1.

I don't know that this is related to this change - it is processing an invalid public key (a failure check) and failing in checking the failure matches ... hard to follow the test failure output logic ...

levitte commented 1 year ago

Note that a pyca test is failing currently with this PR. This could potentially change my vote to -1.

I don't know that this is related to this change - it is processing an invalid public key (a failure check) and failing in checking the failure matches ... hard to follow the test failure output logic ...

That test fails because it expected to see this error:

error:0800006A:elliptic curve routines::point at infinity

With #19681, they are now getting an error that (correctly!) says that the key is invalid. Evidence for this:

$ openssl asn1parse -i -dump <<_____
-----BEGIN PUBLIC KEY-----
MBkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDAgAA
-----END PUBLIC KEY-----
_____
    0:d=0  hl=2 l=  25 cons: SEQUENCE          
    2:d=1  hl=2 l=  19 cons:  SEQUENCE          
    4:d=2  hl=2 l=   7 prim:   OBJECT            :id-ecPublicKey
   13:d=2  hl=2 l=   8 prim:   OBJECT            :prime256v1
   23:d=1  hl=2 l=   2 prim:  BIT STRING        
      0000 - 00 00                                             ..

As you can see for yourselves, the initial byte of the bit string isn't one of the acceptable conversion identities.

romen commented 1 year ago

I still have to find the time do debug this, but what you already checked is quite interesting!

Now I am wondering why this PR changes this, and what are the potential implications that before this PR things went differently!

levitte commented 1 year ago

Shall we discuss that in the PR?

UPDATE: https://github.com/openssl/openssl/pull/19681#discussion_r1024888185

beldmit commented 1 year ago

Vote: [+1]

hlandau commented 1 year ago

Vote: [+1]

romen commented 1 year ago

I just closed the vote, with the following final tally:

Topic: Accept PR #19681 in 3.1 subject to normal review process
Comment: Fundamentally OTC must decide if in the 3.1 release we should
         honor OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT as set (and
         default to UNCOMPRESSED) in our provider implementations, and
         apply corresponding changes for handling legacy keys.
Proposed by: Nicola Tuveri
Issue link: https://github.com/openssl/technical-policies/issues/59
Public: yes
Opened: 2022-11-15
Closed: 2022-11-27
Accepted:  yes  (for: 7, against: 0, abstained: 2, not voted: 2)

  Dmitry     [+1]
  Matt       [+0]
  Pauli      [ 0]
  Tim        [+1]
  Hugo       [+1]
  Richard    [+1]
  Shane      [+1]
  Tomas      [+1]
  Kurt       [  ]
  Matthias   [  ]
  Nicola     [+1]