Skip to content

Add reason codes with the correct offset for two alerts#24338

Closed
jchampio wants to merge 1 commit intoopenssl:openssl-3.0from
jchampio:dev/no-app-protocol-3.0
Closed

Add reason codes with the correct offset for two alerts#24338
jchampio wants to merge 1 commit intoopenssl:openssl-3.0from
jchampio:dev/no-app-protocol-3.0

Conversation

@jchampio
Copy link
Copy Markdown
Contributor

@jchampio jchampio commented May 6, 2024 •
edited
Loading

note: this is currently based on branch openssl-3.0; see below

Attempts to fix #24300. The current SSL_R_NO_APPLICATION_PROTOCOL value doesn't allow for a correct lookup of the reason string, so add a constant that is equal to SSL_AD_REASON_OFFSET + TLS1_AD_NO_APPLICATION_PROTOCOL. Do the same for SSL_R_PSK_IDENTITY_NOT_FOUND.

Here are my "open items" (edit: all answered here):

  1. I'm pretty sure UNKNOWN_PSK_IDENTITY is going to have the same bug.
  2. I don't understand how this interacts with Write SSL_R alerts to error state to keep updated strings #19950, which removed both of the affected codes from the master branch entirely. (That's why I haven't forward-ported this yet.) Should I just add them back?
  3. What's the best place to put a test for this? I see tests for ExpectedServerAlert during the ALPN exchange, but I don't see any tests for the error message itself.
  4. What should be done with the existing SSL_R_NO_APPLICATION_PROTOCOL code? Does that constant need to remain for compatibility?
Checklist
  • documentation is added or updated
  • tests are added or updated

@nhorman
Copy link
Copy Markdown
Contributor

nhorman commented May 6, 2024

can you rebase this on the master branch please? We will apply tags to the PR to backport it appropriately

@t8m
Copy link
Copy Markdown
Member

t8m commented May 6, 2024

note: this is currently based on branch openssl-3.0; see below

Attempts to fix #24300. The current SSL_R_NO_APPLICATION_PROTOCOL value doesn't allow for a correct lookup of the reason string, so add a constant that is equal to SSL_AD_REASON_OFFSET + TLS1_AD_NO_APPLICATION_PROTOCOL.

Here are my "open items":

1. I'm pretty sure `UNKNOWN_PSK_IDENTITY` is going to have the same bug.

Yes, it looks like so.

2. I don't understand how this interacts with [Write SSL_R alerts to error state to keep updated strings #19950](https://github.com/openssl/openssl/pull/19950), which removed both of the affected codes from the `master` branch entirely. (That's why I haven't forward-ported this yet.) Should I just add them back?

Just add the codes back with the proper SSL_R_ name

3. What's the best place to put a test for this? I see tests for `ExpectedServerAlert` during the ALPN exchange, but I don't see any tests for the error message itself.

Hmm... that would be non-trivial. Perhaps we can allow this without a test case.

4. What should be done with the existing `SSL_R_NO_APPLICATION_PROTOCOL` code? Does that constant need to remain for compatibility?

Yes, just keep it unused.

@t8m
Copy link
Copy Markdown
Member

t8m commented May 6, 2024

can you rebase this on the master branch please? We will apply tags to the PR to backport it appropriately

We would need a separate PR for 3.0/3.1 (which can be this PR) because of the changes on master/3.3/3.2 - please create a new PR that will be applied to these 3 branches.

@t8m t8m added triaged: bug The issue/pr is/fixes a bug branch: 3.0 Applies to openssl-3.0 branch branch: 3.1 Applies to openssl-3.1 (EOL) tests: exempted The PR is exempt from requirements for testing labels May 6, 2024
@jchampio
Add reason codes with the correct offset for two alerts
7cb496d 
Fixes openssl#24300. The current values of SSL_R_NO_APPLICATION_PROTOCOL and
SSL_R_PSK_IDENTITY_NOT_FOUND don't allow for a correct lookup of the
corresponding reason strings.

CLA: trivial
@jchampio jchampio force-pushed the dev/no-app-protocol-3.0 branch from 624cd97 to 7cb496d Compare May 8, 2024 18:08
@jchampio jchampio changed the title Add reason code with the correct offset for NO_APPLICATION_PROTOCOL alert Add reason codes with the correct offset for two alerts May 8, 2024
@jchampio jchampio mentioned this pull request May 8, 2024
Closed
2 tasks
@jchampio
Copy link
Copy Markdown
Contributor Author

jchampio commented May 8, 2024 

1. I'm pretty sure `UNKNOWN_PSK_IDENTITY` is going to have the same bug.

Yes, it looks like so.

I took the liberty to add that one, too; if that's unhelpful I can split it back out or revert it.

4. What should be done with the existing `SSL_R_NO_APPLICATION_PROTOCOL` code? Does that constant need to remain for compatibility?

Yes, just keep it unused.

For the record, it's still referenced on the server side. Same with SSL_R_PSK_IDENTITY_NOT_FOUND, both client and server.

can you rebase this on the master branch please? We will apply tags to the PR to backport it appropriately

We would need a separate PR for 3.0/3.1 (which can be this PR) because of the changes on master/3.3/3.2 - please create a new PR that will be applied to these 3 branches.

Done!

@jchampio
Copy link
Copy Markdown
Contributor Author

jchampio commented May 8, 2024

(Oh, and I got mkerr.pl to kick in this time; hence the new copyright updates in the fixed files.)

@t8m t8m added approval: review pending This pull request needs review by a committer approval: otc review pending cla: trivial One of the commits is marked as 'CLA: trivial' labels May 9, 2024
t8m
t8m approved these changes May 9, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

As most of the changes are result of mkerr.pl. I am OK with CLA: trivial.

@t8m t8m requested a review from mattcaswell May 9, 2024 07:51
@tom-cosgrove-arm
Copy link
Copy Markdown
Contributor

tom-cosgrove-arm commented May 10, 2024

@nhorman You are a committer, so can remove the committer approval: review pending label and mark the review as approval: done, right?

@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels May 10, 2024
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels May 11, 2024
@openssl-machine
Copy link
Copy Markdown
Collaborator

openssl-machine commented May 11, 2024

This pull request is ready to merge

@t8m t8m added the hold: wait for master The pull request must wait for approval of the equivalent change on master label May 13, 2024
@t8m
Copy link
Copy Markdown
Member

t8m commented May 13, 2024

Waiting on #24351 to be merged to the master branch.

@t8m t8m removed the hold: wait for master The pull request must wait for approval of the equivalent change on master label May 14, 2024
openssl-machine pushed a commit that referenced this pull request May 14, 2024
@jchampio @t8m
Add reason codes with the correct offset for two alerts
9e33c9c 
Fixes #24300. The current values of SSL_R_NO_APPLICATION_PROTOCOL and
SSL_R_PSK_IDENTITY_NOT_FOUND don't allow for a correct lookup of the
corresponding reason strings.

CLA: trivial

Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24338)
@t8m
Copy link
Copy Markdown
Member

t8m commented May 14, 2024

Merged to the 3.0 and 3.1 branches. Thank you for your contribution.

@t8m t8m closed this May 14, 2024
openssl-machine pushed a commit that referenced this pull request May 14, 2024
@jchampio @t8m
Add reason codes with the correct offset for two alerts
bebc760 
Fixes #24300. The current values of SSL_R_NO_APPLICATION_PROTOCOL and
SSL_R_PSK_IDENTITY_NOT_FOUND don't allow for a correct lookup of the
corresponding reason strings.

CLA: trivial

Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24338)

(cherry picked from commit 9e33c9c)
@jchampio jchampio deleted the dev/no-app-protocol-3.0 branch June 4, 2024 05:17
richardlau added a commit to richardlau/node-1 that referenced this pull request Jun 6, 2024
@richardlau
test: update tests for OpenSSL 3.0.14
fecfde6 
Starting from OpenSSL 3.0.14, 3.1.6, 3.2.2, and 3.3.1, OpenSSL was fixed
to return an error reason string for bad/unknown application protocols.

Update tests to handle both the old `ECONNRESET` error on older versions
of OpenSSL and the new `ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL` on
newer versions of OpenSSL.

Refs: openssl/openssl#24338
@richardlau richardlau mentioned this pull request Jun 6, 2024
Merged
richardlau added a commit to richardlau/node-1 that referenced this pull request Jun 7, 2024
@richardlau
test: update tests for OpenSSL 3.0.14
25d62ec 
Starting from OpenSSL 3.0.14, 3.1.6, 3.2.2, and 3.3.1, OpenSSL was fixed
to return an error reason string for bad/unknown application protocols.

Update tests to handle both the old `ECONNRESET` error on older versions
of OpenSSL and the new `ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL` on
newer versions of OpenSSL.

Refs: openssl/openssl#24338
nodejs-github-bot pushed a commit to nodejs/node that referenced this pull request Jun 10, 2024
@richardlau
test: update tests for OpenSSL 3.0.14
14863e8 
Starting from OpenSSL 3.0.14, 3.1.6, 3.2.2, and 3.3.1, OpenSSL was fixed
to return an error reason string for bad/unknown application protocols.

Update tests to handle both the old `ECONNRESET` error on older versions
of OpenSSL and the new `ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL` on
newer versions of OpenSSL.

Refs: openssl/openssl#24338
PR-URL: #53373
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@sebastianas sebastianas mentioned this pull request Jun 11, 2024
Merged
targos pushed a commit to nodejs/node that referenced this pull request Jun 20, 2024
@richardlau @targos
test: update tests for OpenSSL 3.0.14
602b9d6 
Starting from OpenSSL 3.0.14, 3.1.6, 3.2.2, and 3.3.1, OpenSSL was fixed
to return an error reason string for bad/unknown application protocols.

Update tests to handle both the old `ECONNRESET` error on older versions
of OpenSSL and the new `ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL` on
newer versions of OpenSSL.

Refs: openssl/openssl#24338
PR-URL: #53373
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
@richardlau
test: update tests for OpenSSL 3.0.14
53d1d6e 
Starting from OpenSSL 3.0.14, 3.1.6, 3.2.2, and 3.3.1, OpenSSL was fixed
to return an error reason string for bad/unknown application protocols.

Update tests to handle both the old `ECONNRESET` error on older versions
of OpenSSL and the new `ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL` on
newer versions of OpenSSL.

Refs: openssl/openssl#24338
PR-URL: nodejs#53373
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@richardlau @bmeck
test: update tests for OpenSSL 3.0.14
b15b3e9 
Starting from OpenSSL 3.0.14, 3.1.6, 3.2.2, and 3.3.1, OpenSSL was fixed
to return an error reason string for bad/unknown application protocols.

Update tests to handle both the old `ECONNRESET` error on older versions
of OpenSSL and the new `ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL` on
newer versions of OpenSSL.

Refs: openssl/openssl#24338
PR-URL: nodejs#53373
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Jun 29, 2024
@jchampio @bernd-edlinger
Add reason codes with the correct offset for two alerts
ae078c2 
Fixes openssl#24300. The current values of SSL_R_NO_APPLICATION_PROTOCOL and
SSL_R_PSK_IDENTITY_NOT_FOUND don't allow for a correct lookup of the
corresponding reason strings.

CLA: trivial

Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#24338)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Jun 30, 2024
@jchampio @bernd-edlinger
Add reason codes with the correct offset for two alerts
6f17bf7 
Fixes openssl#24300. The current values of SSL_R_NO_APPLICATION_PROTOCOL and
SSL_R_PSK_IDENTITY_NOT_FOUND don't allow for a correct lookup of the
corresponding reason strings.

CLA: trivial

Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#24338)
marco-ippolito pushed a commit to nodejs/node that referenced this pull request Jul 19, 2024
@richardlau @marco-ippolito
test: update tests for OpenSSL 3.0.14
38f8ee8 
Starting from OpenSSL 3.0.14, 3.1.6, 3.2.2, and 3.3.1, OpenSSL was fixed
to return an error reason string for bad/unknown application protocols.

Update tests to handle both the old `ECONNRESET` error on older versions
of OpenSSL and the new `ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL` on
newer versions of OpenSSL.

Refs: openssl/openssl#24338
PR-URL: #53373
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
marco-ippolito pushed a commit to nodejs/node that referenced this pull request Jul 19, 2024
@richardlau @marco-ippolito
test: update tests for OpenSSL 3.0.14
059e47c 
Starting from OpenSSL 3.0.14, 3.1.6, 3.2.2, and 3.3.1, OpenSSL was fixed
to return an error reason string for bad/unknown application protocols.

Update tests to handle both the old `ECONNRESET` error on older versions
of OpenSSL and the new `ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL` on
newer versions of OpenSSL.

Refs: openssl/openssl#24338
PR-URL: #53373
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@richardlau richardlau mentioned this pull request Aug 21, 2024
Closed
richardlau added a commit to nodejs/node that referenced this pull request Sep 19, 2024
@richardlau
test: update tests for OpenSSL 3.0.14
9f521f4 
Starting from OpenSSL 3.0.14, 3.1.6, 3.2.2, and 3.3.1, OpenSSL was fixed
to return an error reason string for bad/unknown application protocols.

Update tests to handle both the old `ECONNRESET` error on older versions
of OpenSSL and the new `ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL` on
newer versions of OpenSSL.

Refs: openssl/openssl#24338
PR-URL: #53373
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Maxlsc added a commit to Maxlsc/Tongsuo that referenced this pull request Apr 23, 2026
@Maxlsc
Add reason codes with the correct offset for two alerts
ef4d9e2 
The current values of SSL_R_NO_APPLICATION_PROTOCOL and
SSL_R_PSK_IDENTITY_NOT_FOUND don't allow for a correct lookup of the
corresponding reason strings.

Merged from openssl/openssl#24338
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nhorman nhorman nhorman approved these changes

@t8m t8m t8m approved these changes

@mattcaswell mattcaswell Awaiting requested review from mattcaswell

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: 3.0 Applies to openssl-3.0 branch branch: 3.1 Applies to openssl-3.1 (EOL) cla: trivial One of the commits is marked as 'CLA: trivial' tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jchampio @nhorman @t8m @tom-cosgrove-arm @openssl-machine