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

Add function to use TLS when pushing to gateway #994

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Roymprog
Copy link

Closes #982

Copy link
Member

@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

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

Rather than create a second function could we add another argument for verify_mode to tls_auth_handler?

@Roymprog
Copy link
Author

Roymprog commented Feb 2, 2024

We could, the reason I didn't do that was the name of the function. The name tls_auth_handler implies it does authentication. The function becomes confusing if you allow to not provide your own TLS certificate and only use it to verify the certificate of the server.

I'd propose keeping one function called tls_handler, but that would be a breaking change.

Copy link
Member

@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

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

Ahh right, that makes sense, and I think having an auth specific handler is nice.

What would this function provide over just using an https scheme in push_to_gateway? I guess the use case is for if you need to specify the cafile and verify mode?

@Roymprog
Copy link
Author

Roymprog commented Feb 5, 2024

What would this function provide over just using an https scheme in push_to_gateway? I guess the use case is for if you need to specify the cafile and verify mode?

That can be done. It's how I'm doing it right now. If you want to go that route, it would be better to make _make_handler a public function. Otherwise users would need to copy the boilerplate code from that function to create the handler. HTTPShandler cannot be passed to push_to_gateway directly, as it expects a callable (which HTTPShandler) is not.

@csmarchbanks
Copy link
Member

I might be missing something as well, but perhaps this is a bug? I believe push_to_gateway is supposed to just work for https, and you are saying that you need to pass a custom handler to make it work? I wonder if we just need to use the scheme of the url to choose HTTP vs HTTPS in default_handler?

@Roymprog
Copy link
Author

The default_handler works with HTTPS by default. The issue I have is that I need to use a custom SSL context because I need to load a custom trust location. I cannot do that through the default_handler. So, it's unnecessary to specify the scheme.
What would be nice in that case is to have an optional SSL context argument for the default_handler.

@csmarchbanks
Copy link
Member

That makes sense, thank you.

Adding a custom context to default_handler as you suggest seems like it could be a good, and more general solution? That way users could customize the context as they need without us supporting every argument.

I am still thinking about it some and happy to hear other opinions, or advantages/disadvantages of that approach to what you have here. My main concern with this PR is that having two tls related handlers will be confusing.

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.

Allow tls_auth_handler to support client side verification only
2 participants