GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — tls: Add support for ALPN fallback when no ALPN protocol matches by pimterry · Pull Request #45075 · nodejs/node

Conversation

@pimterry

@pimterry pimterry commented Oct 19, 2022

Copy link
Copy Markdown
Member

Fixes #45056.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Oct 19, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pimterry
pimterry force-pushed the alpn-wildcard branch from c906151 to de6f3d5 Compare October 19, 2022 16:17
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Oct 19, 2022
@pimterry
tls: add support for ALPN fallback when no ALPN protocol matches
eccdfd0
@pimterry
pimterry force-pushed the alpn-wildcard branch from de6f3d5 to eccdfd0 Compare October 19, 2022 16:27
@pimterry pimterry mentioned this pull request Oct 19, 2022
Allow ALPN fallback #45056
Closed
@bnoordhuis

bnoordhuis commented Oct 19, 2022

Copy link
Copy Markdown
Member

This PR conflicts with #44875 (sorry, I thought it'd been merged) and since that PR optimizes the common case and this PR adds an uncommon case, I'm going to have to ask you to rebase on top of that.

I would have asked you to take another approach anyway because w->object()->Get(..) is too slow, it's much more efficient to store it on the C++ side in TLSWrap.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell reviewed Oct 19, 2022
View reviewed changes
Comment thread doc/api/tls.md
protocol name. Passing an array is usually much simpler, e.g.
`['hello', 'world']`. (Protocols should be ordered by their priority.)
* `allowALPNFallback`: {boolean} If `true`, if the `ALPNProtocols` option was
set and the client uses ALPN, but requests only protocols that are not do

@jasnell jasnell Oct 19, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Awkwardly structured sentence / grammar here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jasnell
jasnell reviewed Oct 19, 2022
View reviewed changes
Comment thread src/crypto/crypto_tls.cc
->Get(env->context(), env->allow_alpn_fallback_string())
.ToLocalChecked()
.As<Boolean>()
->Value();

@jasnell jasnell Oct 19, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

As @bnoordhuis mentions, this is going to be slow. Definitely should avoid uses of ->Get(...)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pimterry pimterry Oct 20, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'm not very familiar with the JS/C++ interface. What's the preferred way to set a value from the JS TLS wrap such that it's directly accessible here?

If I create a new private field & setter on the C++ wrap, call that from JS in setup, and then read w->allow_alpn_fallback here instead of these calls, is that a) idiomatic and b) sufficiently performant?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@bnoordhuis bnoordhuis Oct 20, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yes and yes. :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pimterry reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@pimterry

pimterry commented Oct 20, 2022

Copy link
Copy Markdown
Member Author

Happy to do the final rebase & changes here, but I'm going to wait for a conclusion on the the callback question in #45056 (comment) first, since that would invalidate this.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pimterry pimterry mentioned this pull request Oct 26, 2022
tls: add ALPNCallback server option for dynamic ALPN negotiation #45190
Merged
@Trott
Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49
@pimterry

pimterry commented Jun 28, 2023

Copy link
Copy Markdown
Member Author

Conclusion from #45056 & #45190 and related discussion seems to be that #45190 is a good change that's strictly better than this, so I'm going to close this now as no longer unnecessary.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pimterry pimterry closed this Jun 28, 2023
@pimterry pimterry mentioned this pull request Sep 14, 2026
quic: add Http3Session, so you can explicitly pick the app protocol #65993
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments
@jasnell jasnell jasnell left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow ALPN fallback

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@pimterry @nodejs-github-bot @bnoordhuis @jasnell
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.