Skip to content

Enable TLS-PSK auth#316

Closed
Elzor wants to merge 1 commit into
ninenines:masterfrom
Elzor:master
Closed

Enable TLS-PSK auth#316
Elzor wants to merge 1 commit into
ninenines:masterfrom
Elzor:master

Conversation

@Elzor
Copy link
Copy Markdown
Contributor

@Elzor Elzor commented Apr 1, 2021

If we need only TLS-PSK authentication (without additional certificates, sni, etc) cowboy returns {error, no_cert} for cowboy:start_tls/3. Actually this is regular way to use PSK, althouht it's rare.
Sample config for this case:

%{
  num_acceptors: 4,
  max_connections: 1024,
  socket_opts: [
    {:protocol, :tls},
    {:port, 12123},
    {:handshake, :full},
    {:ciphers,
      [
        %{cipher: :aes_256_gcm, key_exchange: :psk, mac: :aead, prf: :sha384}
      ]},
    {:user_lookup_fun, {&Hota.Api.Http.PskLookup.lookup/3, <<>>}}
  ]
}

Copy link
Copy Markdown
Member

@essen essen left a comment

Choose a reason for hiding this comment

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

Please add test(s) to acceptor_SUITE. You can make it look similar to the SNI tests.

Comment thread src/ranch_ssl.erl Outdated
@Elzor
Copy link
Copy Markdown
Contributor Author

Elzor commented Apr 2, 2021

@essen Thanks for review and your comments. I've made alphabetical order for options and added two tests for PSK.

@essen
Copy link
Copy Markdown
Member

essen commented Apr 2, 2021

@juhlig @Maria-12648430 Can you please review? Thanks!

@juhlig
Copy link
Copy Markdown
Contributor

juhlig commented Aug 27, 2021

Oh... I never noticed the review request =^^= SSL/TLS is really not my home ground, but in any case, the tests on buildkite mostly failed in just the tests added by this PR, so...

@essen
Copy link
Copy Markdown
Member

essen commented Aug 31, 2021

@Maria-12648430 I think merging this wouldn't be a problem for #324 (minus a potential rebase), since we move forward when the option is there, and if using TLS 1.3 we then reject the option. Correct?

@Maria-12648430
Copy link
Copy Markdown
Contributor

@Maria-12648430 I think merging this wouldn't be a problem for #324 (minus a potential rebase), since we move forward when the option is there, and if using TLS 1.3 we then reject the option. Correct?

Hm, I'm not sure. There is some kind of clash here, yes. If the user_lookup_fun option is present, we proceed here, then (with #324) iff TLSv1.3 is the only protocol version used, it is erased again. OTOH, user_lookup_fun is not usable with (only) TLSv1.3, anyway, and currently (without #324) the listener won't start at all but fail with an option dependency error.

@essen
Copy link
Copy Markdown
Member

essen commented Aug 31, 2021

Right we remove not reject. So we would end up with a potentially confusing error then I think. But I'm not sure we should do anything about it.

@Maria-12648430
Copy link
Copy Markdown
Contributor

I have no idea what would happen if user_lookup_fun was used in a setup containing TLSv1.3 and, say, v1.2 (which ssl and subsequently ranch_ssl allow)), and the actual connection was then made with TLSv1.3 ^^;;;

@Maria-12648430
Copy link
Copy Markdown
Contributor

Right we remove not reject. So we would end up with a potentially confusing error then I think. But I'm not sure we should do anything about it.

The possible outcomes for [{user_lookup_fun, ...}, {versions, ['tlsv1.3'}] are...

@essen
Copy link
Copy Markdown
Member

essen commented Aug 31, 2021

OK good enough.

@essen essen added this to the 2.1.0 milestone Sep 1, 2021
Comment thread test/acceptor_SUITE.erl
@essen
Copy link
Copy Markdown
Member

essen commented Sep 2, 2021

Merged, thanks!

@essen essen closed this Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants