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

Change TLS to conditionally load native certs #794

Merged
merged 1 commit into from
Jan 5, 2023
Merged

Change TLS to conditionally load native certs #794

merged 1 commit into from
Jan 5, 2023

Conversation

Jarema
Copy link
Member

@Jarema Jarema commented Jan 4, 2023

Until now, when TLS was used, native OS certs were always loaded. This should not be the case if CA is provided in rustls config or options.

@Jarema Jarema requested a review from caspervonb January 4, 2023 12:49
@Jarema Jarema force-pushed the jarema/conditionally-load-tls branch from 0c3682b to ab1a21c Compare January 4, 2023 14:03
@caspervonb caspervonb force-pushed the jarema/conditionally-load-tls branch from 7abbe56 to 8236fc4 Compare January 4, 2023 16:34
@caspervonb
Copy link
Collaborator

caspervonb commented Jan 4, 2023 •
edited
Loading

Full server logs from failure:

[95350] 2023/01/04 19:54:35.739705 [INF] Listening for client connections on 0.0.0.0:4222
[95350] 2023/01/04 19:54:35.739716 [DBG] Get non local IPs for "0.0.0.0"
[95350] 2023/01/04 19:54:35.739916 [DBG]   ip=192.168.10.120
[95350] 2023/01/04 19:54:35.739921 [DBG]   ip=2a01:799:1b9f:1600:18c8:e300:1aca:451
[95350] 2023/01/04 19:54:35.739924 [DBG]   ip=2a01:799:1b9f:1600:816a:358b:d267:1531
[95350] 2023/01/04 19:54:35.740018 [INF] Server is ready
[95350] 2023/01/04 19:54:35.740025 [DBG] maxprocs: Leaving GOMAXPROCS=8: CPU quota undefined
[95350] 2023/01/04 19:54:43.809860 [DBG] 127.0.0.1:59080 - cid:4 - Client connection created
[95350] 2023/01/04 19:54:44.245401 [TRC] 127.0.0.1:59080 - cid:4 - ->> [-ERR Unknown Protocol Operation]
[95350] 2023/01/04 19:54:44.245439 [ERR] 127.0.0.1:59080 - cid:4 - Client parser ERROR, state=0, i=0: proto='"\x16\x03\x01\x00\xee\x01\x00\x00\xea\x03\x03F\x1f:Dh\xb0\ue850\x1cQ\x9di=\x1a\x12\x16\x11f\xac("...'
[95350] 2023/01/04 19:54:44.245450 [DBG] 127.0.0.1:59080 - cid:4 - Client connection closed: Protocol Violation

The difference you're seeing @Jarema between local and CI is from /etc/hosts redirection.

@ronz-sensible
Copy link

@caspervonb , I just test it with basic_tls test and found that your addition was not necessary and cause the test to fail.

@caspervonb caspervonb force-pushed the jarema/conditionally-load-tls branch from 8236fc4 to ab1a21c Compare January 5, 2023 09:22
@caspervonb
Copy link
Collaborator

Had to test, reverted 😁

@Jarema
Change TLS to conditionally load native certs
94f8974 
Until now, when TLS was used, native OS certs were always loaded.
This should not be the case if CA is provided in rustls config
or options.
@Jarema Jarema force-pushed the jarema/conditionally-load-tls branch from ab1a21c to 94f8974 Compare January 5, 2023 09:51
caspervonb
caspervonb approved these changes Jan 5, 2023
View reviewed changes
Copy link
Collaborator

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

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

LGTM

@Jarema Jarema merged commit 33deb4c into main Jan 5, 2023
@Jarema Jarema deleted the jarema/conditionally-load-tls branch January 5, 2023 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caspervonb caspervonb caspervonb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Jarema @caspervonb @ronz-sensible