-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Allow longer username and password under Dynamic Challenge/Response P… #295
base: master
Are you sure you want to change the base?
Conversation
Related to previous work at #273. |
This is missing documentation and also some justification why this change is relevant and how applications are going to use these changes. |
Changing username/password buffer size to 64K looks like a wrong approach to me. Can't this be done better using webauth and opening a url? |
I lifted this from a patch that vendors are already making. For whatever
reason, they chose to push SAML through the password option rather than use
webauth. Maybe that didn’t exist at the time? In any case, that’s the
reasoning behind this patch.
…On Wed, Mar 22, 2023 at 15:26 Selva Nair ***@***.***> wrote:
Changing username/password buffer size to 64K looks like a wrong approach
to me. Can't this be done better using webauth and opening a url?
—
Reply to this email directly, view it on GitHub
<#295 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAESAMUA2B5DOI2ENFU4JTW5N4ATANCNFSM6AAAAAAWEGZS2Y>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
*/ | ||
#define TLS_CHANNEL_BUF_SIZE 2048 | ||
#define TLS_CHANNEL_BUF_SIZE 65536 |
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 don't think this makes sense as TLS record size is only 16k.
* Increase the username and password length size to 65KB, in order | ||
* to support long passwords under the dynamic challenge/response protocol. | ||
*/ | ||
#define USER_PASS_LEN 65536 |
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.
Only increasing the size to 65k when PKCS11 is not enabled feels questionable to say the least. This would in most cases not change anything as PKCS11 is typically enabled.
#define OPTION_PARM_SIZE 256 | ||
#define OPTION_LINE_SIZE 256 | ||
#define OPTION_PARM_SIZE USER_PASS_LEN | ||
#define OPTION_LINE_SIZE OPTION_PARM_SIZE |
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.
Changing these constants here is a very drastic change that changes all kind of parsing.
What vendors? And how is this used? If you cannot even explain how this is used or why these changes are useful for anyone, I have a hard time seeing any value in this patch. |
@schwabe For example AWS VPN Client, when making the authentication with SAML it sends |
In this blog you will have more context of what we are talking about: https://smallhacks.wordpress.com/2020/07/08/aws-client-vpn-internals/ @schwabe do you have a better way to support more length in the password argument? Unfortunately I don't have any experience with C/C++ and if you say this is not the right way to implement it I believe you. However I ask you please if you can provide us a way to solve this scenario since you have more insight in OpenVPN. |
So it is basically a hacky method supported only by AWS. I am not really sure I want to bring that hack into the main OpenVPN code base. |
Yes we can call it like that 😅 I'm thinking if we can support longer passwords lengths via arguments. What do you think? The default behavior will remain for OpenVPN but for this case we can do e.g: openvpn --password-length="bytes" So will work for AWS and perhaps other vendors that follow this scenario. |
@schwabe it's been sometime we talked about this, do you think the arguments approach would be possible? |
The question is still why should support this hacky way that is not even documented that is properitary to OpenVPN when we have a well documented way to do this properly. |
Is it documented already? For passing a long password? Could you show me please? @schwabe |
@sant123 there is no reason to pass such a super long password apart from the hacky AWS client. We have better methods than that to support different autehtnication methods like SAML. |
Yes I understand that, but that’s how AWS implemented it’s system authentication. If there would be other way believe me I’ll take it but this is how is implemented now. They ask for a long password and that’s now how is currently working for @m4dc4p, other people and myself. Don’t know other vendors but wanted to have another way to configure these kind of scenarios @schwabe |
Yeah, but I don't think the Lemming approach here the way to go. Just because someone else jumped of a cliff, does not mean we have to follow and jump off the same cliff too. AWS did something to the OpenVPN protocol, which we do not want to support since there are multiple bad things about their approach that break compatiblity with non-AWS servers and while AWS does not care, we care about compatiblity. And I won't break compatibility with the current clients just to have AWS hack in OpenVPN. |
That's fair enough. Thanks for having the patience to explaining us this 👍🏼 |
…rotocol. Based on patches found at https://github.com/samm-git/aws-vpn-client, this updates OpenVPN for compatibility with AWS' (and other vendors) use of the dynamic challenge/response protocol to implement SAML-based authentication. Those vendors submit the password via the management interface, which can be up to 50kb long.
In context of https://github.com/jkroepke/openvpn-auth-oauth2/ I'm also interest into this. Mainly increase auth-token, because I would like to pass the ID Token from OIDC Provider to VPN clients to allow a full stateless SSO experience. ID Tokens can be large (up to 8K). |
…rotocol.
Based on patches found at https://github.com/samm-git/aws-vpn-client, this updates OpenVPN for compatibility with AWS' (and other vendors) use of the dynamic challenge/response protocol to implement SAML-based authentication.
Those vendors submit the password via the management interface, which can be up to 50kb long.