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.

Sign up for GitHub

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

Jump to bottom

tls_client_auth #328

Merged
merged 7 commits into from
Jan 10, 2024
Merged

tls_client_auth #328

merged 7 commits into from
Jan 10, 2024

Conversation

paulswartz
Copy link
Collaborator

Fixes: #318 #327

I think even this attempt with the profile is still a bit flaky. I need to figure out if the errors I'm seeing with the conformance suite are due to race conditions here, or something else.

@paulswartz
feat: create an httpc profile which disables keep-alive/pipelining
014e374 
As noted in
erlef#318 (comment),
`httpc` by default will re-use existing connections. This is great if
you're using normal HTTPS, but if you're using client authentication
then you need to make sure that every time `httpc` connects to a host,
it's using the client authentication, which is impossible in practice.

This works around that, by creating a new profile which disables that
functionality. Using that profile for requests which provide SSL
overrides will ensure that each of those requests will use the client certificate.
@paulswartz paulswartz marked this pull request as draft January 8, 2024 03:03
@paulswartz
feat: tell httpc to close the connection if we're using client certs
a23417d 
In addition to the separate profile, this header serves as a note to
both the remote server and `httpc` that we're not keeping the connection
open after the request.`
@maennchen
Copy link
Member

maennchen commented Jan 8, 2024 •
edited
Loading

I was thinking to go more of a dynamic supervisor approach instead of the short keepalive one:

  • Calculate hash of config
  • Start profile for it if it does not yet exist
  • Use profile
  • Automatically stop profiles if not used for x time (not sure if we need this)

@paulswartz paulswartz linked an issue Jan 8, 2024 that may be closed by this pull request
FAPI 2.0: enforce TLS 1.2 and specific ciphers #327
Closed
@maennchen
Copy link
Member

A slightly simpler option is to just delegate this to the user. In normal circumstances it is enough to provide the tls client certificate to all request_opts.

For cases like our tests and also for users that use multiple issuers on the same domain, we can just tell them to create inets profiles and add a profile option to request_opts.

@paulswartz
Copy link
Collaborator Author

paulswartz commented Jan 10, 2024 •
edited
Loading

Love that simpler option, and it works for the conformance tests.

.......................................................................................................................................................
Finished in 166.5 seconds (166.5s async, 0.00s sync)
151 tests, 0 failures

@paulswartz paulswartz marked this pull request as ready for review January 10, 2024 14:12
maennchen
maennchen reviewed Jan 10, 2024
View reviewed changes
src/oidcc_app.erl Outdated Show resolved Hide resolved
@paulswartz paulswartz requested a review from maennchen January 10, 2024 15:06
@maennchen maennchen merged commit 97dcc2a into erlef:main Jan 10, 2024
24 of 25 checks passed
@maennchen maennchen added this to the v3.2.0 milestone Jan 10, 2024
@maennchen maennchen self-assigned this Jan 10, 2024
@paulswartz paulswartz deleted the tls-client-auth branch January 13, 2024 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maennchen maennchen maennchen approved these changes

Assignees

@maennchen maennchen

Labels
enhancement
Projects
None yet
Milestone
v3.2.0
Development

Successfully merging this pull request may close these issues.

FAPI 2.0: enforce TLS 1.2 and specific ciphers tls_client_auth authentication mode
2 participants
@paulswartz @maennchen