-
Notifications
You must be signed in to change notification settings - Fork 383
T7635: OpenConnect Certificate Authentication #4618
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: current
Are you sure you want to change the base?
Conversation
👍 |
I believe this would be backwards compatible to Sagitta if you'd like to backport it. |
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.
Pull Request Overview
This PR implements certificate-based authentication for OpenConnect VPN by adding support for client certificate validation. The implementation allows users to configure certificate authentication using Common Name (CN) or UID fields from client certificates.
- Adds certificate authentication mode with support for CN and UID certificate fields
- Updates authentication mode validation to handle the new certificate option
- Requires CA certificate configuration when using certificate authentication
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
src/conf_mode/vpn_openconnect.py | Adds certificate authentication validation logic and mutual exclusion checks |
interface-definitions/vpn_openconnect.xml.in | Defines the new certificate authentication configuration option |
data/templates/ocserv/ocserv_config.j2 | Updates the ocserv configuration template to output certificate authentication settings |
src/conf_mode/vpn_openconnect.py
Outdated
'local' in ocserv['authentication']['mode'] | ||
and 'radius' in ocserv['authentication']['mode'] | ||
or | ||
'local' in ocserv['authentication']['mode'] | ||
and 'cert' in ocserv['authentication']['mode'] | ||
or | ||
'radius' in ocserv['authentication']['mode'] | ||
and 'cert' in ocserv['authentication']['mode'] |
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.
The logical operators 'or' and 'and' have different precedence, which may cause unexpected behavior. The condition should be properly grouped with parentheses to ensure correct evaluation: or ('local' in ocserv['authentication']['mode'] and 'cert' in ocserv['authentication']['mode'])
'local' in ocserv['authentication']['mode'] | |
and 'radius' in ocserv['authentication']['mode'] | |
or | |
'local' in ocserv['authentication']['mode'] | |
and 'cert' in ocserv['authentication']['mode'] | |
or | |
'radius' in ocserv['authentication']['mode'] | |
and 'cert' in ocserv['authentication']['mode'] | |
('local' in ocserv['authentication']['mode'] | |
and 'radius' in ocserv['authentication']['mode']) | |
or | |
('local' in ocserv['authentication']['mode'] | |
and 'cert' in ocserv['authentication']['mode']) | |
or | |
('radius' in ocserv['authentication']['mode'] | |
and 'cert' in ocserv['authentication']['mode']) |
Copilot uses AI. Check for mistakes.
src/conf_mode/vpn_openconnect.py
Outdated
ocserv['authentication']['mode']['cert'] = '2.5.4.3' | ||
elif 'uid' in ocserv['authentication']['mode']['cert']: | ||
ocserv['authentication']['mode']['cert'] = '0.9.2342.19200300.100.1.1' |
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.
This code modifies the configuration dictionary during verification, which is incorrect. The verification function should only validate the configuration, not modify it. The OID mapping should be handled in the template or a separate processing step.
ocserv['authentication']['mode']['cert'] = '2.5.4.3' | |
elif 'uid' in ocserv['authentication']['mode']['cert']: | |
ocserv['authentication']['mode']['cert'] = '0.9.2342.19200300.100.1.1' | |
pass # Validation logic for 'cn' can be added here if needed | |
elif 'uid' in ocserv['authentication']['mode']['cert']: | |
pass # Validation logic for 'uid' can be added here if needed |
Copilot uses AI. Check for mistakes.
src/conf_mode/vpn_openconnect.py
Outdated
ocserv['authentication']['mode']['cert'] = '2.5.4.3' | ||
elif 'uid' in ocserv['authentication']['mode']['cert']: | ||
ocserv['authentication']['mode']['cert'] = '0.9.2342.19200300.100.1.1' |
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.
This code modifies the configuration dictionary during verification, which is incorrect. The verification function should only validate the configuration, not modify it. The OID mapping should be handled in the template or a separate processing step.
ocserv['authentication']['mode']['cert'] = '2.5.4.3' | |
elif 'uid' in ocserv['authentication']['mode']['cert']: | |
ocserv['authentication']['mode']['cert'] = '0.9.2342.19200300.100.1.1' | |
pass # OID mapping will be handled in a separate step | |
elif 'uid' in ocserv['authentication']['mode']['cert']: | |
pass # OID mapping will be handled in a separate step |
Copilot uses AI. Check for mistakes.
<description>Custom OID in dotted decimal format</description> | ||
</valueHelp> | ||
<constraint> | ||
<regex>(^\.?\d{1,5}(?:\.\d{1,5})*$|cn|uid)</regex> |
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.
The regex pattern allows OIDs starting with a dot (e.g., '.1.2.3'), but standard OID notation should not start with a dot. The pattern should be (^\d{1,5}(?:\.\d{1,5})*$|cn|uid)
to only allow valid OID formats.
<regex>(^\.?\d{1,5}(?:\.\d{1,5})*$|cn|uid)</regex> | |
<regex>(^\d{1,5}(?:\.\d{1,5})*$|cn|uid)</regex> |
Copilot uses AI. Check for mistakes.
ae471d3
to
96d74db
Compare
Updates made based off Copilot review feedback. |
<help>Use certificate based authentication</help> | ||
<valueHelp> | ||
<format>cn</format> | ||
<description>OID 2.5.4.3 - Common Name</description> |
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.
My feeling is that these should be sub-nodes like cert cn oc.example.com
. That way it would be more obvious to config readers what field of the cert it's supposed to match, and it will be possible to add separate constraints for CNs and UIDs.
I also don't know if help strings should mention OIDs. End users normally talk about symbolic names like CN, I can't see how OIDs could be important or useful here in help strings.
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.
What about using certificate
instead of cert
?
And common-name
instead of cn
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.
The actual config is the OID, not a name. I don't think you can have more than one selection. I was referencing the OIDs because that's how the example config references that config item. So I don't think putting a domain name in there would be useful, and might lead to confusion by the user.
The OID definition is what openconnect should list as the username in the output of the show commands, and be used for any user specific configurations. The server will validate the user's certificate against the CA that's defined in the configuration to make sure the certificate is valid.
I suppose I figured seeing OIDs in the help might key in a more advanced user that they can certainly specify a specific OID for the configuration to match on. I think CN is going to be the most common use case, but the two listed in the openconnect example configs were CN and UID.
I don't see any issue using certificate instead of cert. I will make that change at a minimum.
I don't have any issue taking out the OID references from the help text if you'd rather them not be in there.
Here's the section from the example config taken from https://github.com/openconnect/ocserv/blob/master/doc/sample.config
# The object identifier that will be used to read the user ID in the client
# certificate. The object identifier should be part of the certificate's DN
# Useful OIDs are:
# CN = 2.5.4.3, UID = 0.9.2342.19200300.100.1.1, SAN(rfc822name)
cert-user-oid = 0.9.2342.19200300.100.1.1
CI integration ❌ failed! Details
|
Change summary
Adds feature to enable certificate based authentication for OpenConnect
Types of changes
Related Task(s)
https://vyos.dev/T7635
Related PR(s)
vyos/vyos-documentation#1662
How to test / Smoketest result
Check ocserv.conf for auth = "certificate" and cert-user-oid = 2.5.4.3
Checklist: