-
-
Notifications
You must be signed in to change notification settings - Fork 492
[18.0][IMP]auth_oidc: verify self-signed certificates #837
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
base: 18.0
Are you sure you want to change the base?
[18.0][IMP]auth_oidc: verify self-signed certificates #837
Conversation
|
Hi @sbidoul, |
ap-wtioit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good (just the missing space is a requirement for the PR to be correct from my point of view)
This is a useful addition for checking with local HTTPS were one cannot really get official certificates.
If the connection between odoo and an oauth provider uses self-signed certificates, a ssl error is thrown because the self-signed certificated cannot be verified.
ad30577 to
02e4c85
Compare
sbidoul
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. A couple a comment after a quick look.
| string="Self-signed", | ||
| help="Disable certificate checks for server to server token requests " | ||
| "when using self signed certificates.", | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this does not make it sufficiently clear that this enables an insecure option. Could we name this field, say, "insecure" like curl does?
| string="Self-signed verify path", | ||
| help="Path to the self-signed certificate for the verification process. " | ||
| "Empty value disables the verification.", | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would name this field ca_bundle, but I have a strong preference to let that be configured at the system level, or with the REQUESTS_CA_BUNDLE environment variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me the benefit here would be that this is only for one OAuth server connection while REQUESTS_CA_BUNDLE is for all requests from Odoo to other servers.
If the connection between odoo and an oauth provider uses self-signed certificates, a ssl error is thrown because the self-signed certificated cannot be verified.
Even if the user credentials are correct, the user will not get logged in.
Following error gets thrown:
requests.exceptions.SSLError: HTTPSConnectionPool(host='', port=443): Max retries exceeded with url: /realms//protocol/openid-connect/token (Caused by SSLError(SSLCertVerificationError(1, '[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self-signed certificate in certificate chain (_ssl.c:1000)')))
Steps to reproduce: