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

Update for SAML (long username/password) support #273

Closed
wants to merge 1 commit into from

Conversation

m4dc4p
Copy link

@m4dc4p m4dc4p commented Mar 9, 2023

Based on patches found at https://github.com/samm-git/aws-vpn-client, this updates OpenVPN for compatibility with AWS' SAML based authentication.

Thank you for your contribution

You are welcome to open PR, but they are used for discussion only. All
patches must eventually go to the openvpn-devel mailing list for review:

Please send your patch using git-send-email. For example to send your latest commit to the list:

$ git send-email [email protected] HEAD~1

For details, see these Wiki articles:

Based on patches found at https://github.com/samm-git/aws-vpn-client,
this updates OpenVPN for compatibility with AWS' SAML based
authentication.
@m4dc4p
Copy link
Author

m4dc4p commented Mar 9, 2023

The repository I linked applies some patches to increase the buffer size used to hold command line arguments, and some others, which ultimately allow openvpn to be used with SAML-based authentication.

If the changes seem solid, it seems to make sense to get them into the official release.

@schwabe
Copy link
Contributor

schwabe commented Mar 9, 2023

Those patches are breaking compatibility with existing OpenVPN servers/clients. There is also no documentation or examples or other indicates how these changes would help with anything.

OpenVPN already supports SAML auth, see management-notes.txt and auth pending support.

@m4dc4p
Copy link
Author

m4dc4p commented Mar 9, 2023

That's fantastic - thanks for the pointers! I'll dig in. Much appreciated.

@m4dc4p m4dc4p closed this Mar 9, 2023
@sant123
Copy link

sant123 commented Mar 22, 2023

@m4dc4p did it work for you?

@schwabe I think it should not be breaking compatibility. Basically is adding more memory to store the SAML response (since it can be large like 11Kb) and send the credentials through the VPN Server.

@m4dc4p
Copy link
Author

m4dc4p commented Mar 22, 2023

@sant123 Yeh, this commit mostly worked for me. What I learned was my vendor is using the "Dynamic Challenge" protocol, and the SAML response is pushed to OpenVPN via the 'password' command in the management interface. To support that, you still need to increase the storage size of the username and password buffer. I've pushed an update with minimal changes needed.

See #295.

@schwabe
Copy link
Contributor

schwabe commented Mar 22, 2023

@sant123 the patch definitively breaks compatiblity as it changes some fields from 2 byte to 4 bytes on the wire protocol without caring about compatibility at all. (e.g the change from if (!buf_write_u16(buf, 0)) to if (!buf_write_u32(buf, 0))).

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.

3 participants