Add TLS Validation #197
Add TLS Validation #197
Conversation
allow specification of: - ca bundle for server verification - client certificate/key for client verification Signed-off-by: Robin Bergewski <robinbergewski@gmail.com>
| def get_auth_backend( | ||
| name="token", session=None, insecure=False, tls_verify=True, **kwargs | ||
| ): | ||
| def get_auth_backend(name="token", session=None, insecure=False, **kwargs): | ||
| backend = auth_backends.get(name) | ||
| if not backend: | ||
| raise ValueError(f"Authentication backend {backend} is not known.") | ||
| backend = backend(**kwargs) | ||
| backend.session = session or requests.Session() |
There was a problem hiding this comment.
This kinda is a problem if someone does not pass in the session... Is this something that needs to be taken into account or can we not just make session a required argument here. Having ONE session which is pre-configured to use for all requests has the advantage that we can pass less arguments around settings like
self.session.cookies.set_policy(DefaultCookiePolicy(allowed_domains=[]))cannot be forgotten
There was a problem hiding this comment.
The one session and passing it forward from the client was the idea - see how it is created and passed forward here:
Line 68 in 9e29e24
There was a problem hiding this comment.
should the function get_auth_backend stay like this since it is correctly used for should session be made a required parameter?
There was a problem hiding this comment.
Sorry I don't understand - do you mean or should session be made required?
There was a problem hiding this comment.
Either we leave it like it is now since it works fine the way it is currently used or we change it to
def get_auth_backend(session: requests.Session, name="token", , insecure=False, **kwargs):so that no that no mistakes be made where in the future somebody does not pass in the session and just does:
get_auth_backend(name="token", insecure=False)
|
The test is failing since I removed the explicit The session has You might have known this, I just learned this today ... 😲 Since the user can now set the CA bundle explicitly, would it make sense to set What are your thoughts? How should we proceed? |
|
If the session is shared, for your implementation can you just create the client and set session.verify = |
|
@vsoch do you mean that this change should not be made and the user of the r = Registry(...)
r.session.verify = '/path/to/my/bundle' |
|
@Sojamann yes I'm wondering if you'd like to try that and see if it works for your use case. |
|
This would also work, but as a user of Oras, I would then be exposed to the internals of the library. |
|
@Sojamann it's a developer library by default - I would argue that your use case is niche and should not refactor the entire structure of the library to support when it's as easy as tweaking the session on the class instance. |
|
Hm I’m not fully on board with this, but I guess it is what it is. I’ll go ahead and close the PR then. |
This PR adds tls verification options and solves #185.
At the moment only insecure (no tls verification) is supported which I'd like to change since I cannot rely on the hosts CA Bundles which requests uses by default.
The tls options are also supported by the oras binary.