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.

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

wip: Add replacer vars for mTLS connection details. #67

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dekimsey
Copy link

This is based on the effort in caddyhttp's replacer but
with added Subject/Issuer details.

Also include a minor update to add the cipher_suite replacer for parity.

May be a wee bit overkill, but we actually do OU tests on incoming connections to validate which component in our stack issued the client cert. It did make me think one could refactor this to be sitting in caddy's caddytls module so both may share the Replacer with corresponding prefixes.

I added a basic test based on the http module but didn't know how to go about mocking the entire connection so some of the clientHello replacer values aren't tested.

This is based on the effort in caddyhttp's replacer but
with added Subject/Issuer details.

Also include a minor update to add the cipher_suite replacer for parity.
@mholt
Copy link
Owner

mholt commented Sep 2, 2022

Hey, thanks for the PR. Sorry I haven't replied yet. Been really busy trying to get Caddy 2.6 ready to go. This is still on my list 🙃

Copy link
Owner

@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.

LGTM. Thank you for the contribution!

Anything else you want to do before I merge?

@dekimsey
Copy link
Author

dekimsey commented Sep 8, 2022

I think I should remove some of those FIXMEs. What would you like me to do with them?

@@ -104,6 +120,209 @@ func (t *Handler) Handle(cx *layer4.Connection, next layer4.Handler) error {
return next.Handle(cx.Wrap(tlsConn))
}

// FIXME: Should this perhaps be moved instead to caddytls?
Copy link
Owner

Choose a reason for hiding this comment

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

Let's start by trying it here first, then it'll be easier to consider moving it upstream. 👍

input string
expect string
}{
// FIXME: These are set during the match phase, but it's unclear how to construct a test that properly
Copy link
Owner

Choose a reason for hiding this comment

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

I probably need to build out more test tools for this module (and Caddy too).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants