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

reverseproxy: Implement modular CA provider for TLS transport #6065

Merged
merged 3 commits into from
Apr 12, 2024
Merged

reverseproxy: Implement modular CA provider for TLS transport #6065

merged 3 commits into from
Apr 12, 2024

Conversation

armadi1809
Copy link
Contributor

Closes #6063

@armadi1809 armadi1809 force-pushed the new-ca-providers-for-reverse-proxy branch from d605170 to ab39dd4 Compare January 26, 2024 05:12
@armadi1809
Copy link
Contributor Author

Looks like the failed test is just a server timeout.

@armadi1809 armadi1809 force-pushed the new-ca-providers-for-reverse-proxy branch from ab39dd4 to c3a6efe Compare January 26, 2024 15:19
mohammed90
mohammed90 requested changes Jan 26, 2024
View reviewed changes
Copy link
Member

@mohammed90 mohammed90 left a comment

Choose a reason for hiding this comment

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

GitHub does not let me comment on the unchanged lines, so excuse the non-inlined comments.

Mark the old way as deprecated and subject to removal in the future, and emit logs saying the same when used. You can get a logger from ctx.Logger().

modules/caddyhttp/reverseproxy/httptransport.go Outdated Show resolved Hide resolved
@armadi1809 armadi1809 force-pushed the new-ca-providers-for-reverse-proxy branch from c3a6efe to 00b1449 Compare January 27, 2024 17:09
@armadi1809
Copy link
Contributor Author

I just added the "deprecated" comments to RootCAPool and RootCAPEMFiles fields as well as a deprecation warning log when those are used.

mohammed90
mohammed90 requested changes Jan 28, 2024
View reviewed changes
Copy link
Member

@mohammed90 mohammed90 left a comment

Choose a reason for hiding this comment

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

Thanks for doing the rounds, Aziz! Sorry for not giving the review points in full at once. I'm squeezing the review time in tight time slots 😅

We're missing tests and Caddyfile support

@armadi1809
Copy link
Contributor Author

No worries at all Mohammed! Thank you for your feedback. I implemented the caddy file support and added both caddy file integration tests as well as tests for the (h *HTTPTransport) UnmarshalCaddyfile function. Please let me know if anything else is still needed here. Thanks again!

mholt
mholt approved these changes Jan 31, 2024
View reviewed changes
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Sorry for falling behind -- this is looking better now! I'd be willing to give this a try. Thank you!!

@mholt
Copy link
Member

mholt commented Mar 6, 2024

@mohammed90 Anything you can think of we need to change for this?

mohammed90
mohammed90 requested changes Mar 8, 2024
View reviewed changes
Copy link
Member

@mohammed90 mohammed90 left a comment

Choose a reason for hiding this comment

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

A few minor nitpicks and I believe I got everything

modules/caddyhttp/reverseproxy/caddyfile.go Show resolved Hide resolved
modules/caddyhttp/reverseproxy/httptransport.go Outdated Show resolved Hide resolved
modules/caddyhttp/reverseproxy/httptransport_test.go Outdated Show resolved Hide resolved
@mohammed90 mohammed90 changed the title Added modular ca providers the tls config of the reverse proxy http transport module. reverseproxy: transport: implement modular CA provider for TLS transport Mar 8, 2024
@armadi1809 armadi1809 force-pushed the new-ca-providers-for-reverse-proxy branch 2 times, most recently from f8ba559 to 5e7a4e7 Compare March 10, 2024 17:04
@armadi1809 armadi1809 force-pushed the new-ca-providers-for-reverse-proxy branch from 5e7a4e7 to 391e352 Compare March 10, 2024 18:53
@armadi1809
Copy link
Contributor Author

@mohammed90 I think all your feedback items have been resolved Thanks for reviewing!

mohammed90
mohammed90 approved these changes Mar 12, 2024
View reviewed changes
Copy link
Member

@mohammed90 mohammed90 left a comment

Choose a reason for hiding this comment

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

LGTM! Waiting on @mholt review then we're good to go.

@francislavoie francislavoie added the feature ⚙️ New feature or request label Mar 12, 2024
@francislavoie francislavoie added this to the v2.8.0 milestone Mar 12, 2024
@francislavoie francislavoie changed the title reverseproxy: transport: implement modular CA provider for TLS transport reverseproxy: Implement modular CA provider for TLS transport Mar 12, 2024
@mholt mholt merged commit 0b381eb into caddyserver:master Apr 12, 2024
25 checks passed
@mohammed90 mohammed90 mentioned this pull request Apr 23, 2024
Merged
@WeidiDeng WeidiDeng mentioned this pull request May 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mholt mholt mholt approved these changes

@mohammed90 mohammed90 mohammed90 approved these changes

Assignees
No one assigned
Labels
feature ⚙️ New feature or request
Projects
None yet
Milestone
v2.8.0
Development

Successfully merging this pull request may close these issues.

reverse proxy: use the new CA providers
4 participants
@armadi1809 @mholt @mohammed90 @francislavoie