Repository navigation
CP-312485: Pool-admin permissions and trusted CAs for client certificate auth - #7360
YizhouChen1998 wants to merge 5 commits into
Conversation
Zhengchai
left a comment
There was a problem hiding this comment.
Regarding " For reviewers: install_ca_certificate, install_trusted_certificate and enable_client_certificate_auth are pool-operator calls, so a pool-operator can now obtain a pool-admin session. Should they require pool-admin? "
Could you pls clarify do you mean Pool.install_ca_certificate' - which XS XAPI plays as Client to connect to remote peers, or or Host.install_server_certifiate' - which XS XAPI plays as Server to accept incoming connections?
| , _R_VM_POWER_ADMIN ++ _R_CLIENT_CERT | ||
| , [] | ||
| ) | ||
| , (Post, Constants.xenops_uri, false, [], _R_VM_POWER_ADMIN, []) |
There was a problem hiding this comment.
this looks like just a format change, why is this required?
There was a problem hiding this comment.
It's not only formatting: the old code had _R_VM_POWER_ADMIN ++ _R_CLIENT_CERT, and this commit drops ++ _R_CLIENT_CERT because the client-cert role is removed.
psafont
left a comment
There was a problem hiding this comment.
The design needs more work, a lot of it is coming straight out of the LLM, which is quite grating. Please review all prose produced by the LLM before it's sent as a PR
| revision: 3 | ||
| status: released (26.13.0) |
There was a problem hiding this comment.
The status needs to be changed so reflect there are some proposed changes that have not been released.
Something along the lines of:
status: released (26.13.0), with proposed changes for releases after 26.22.0
| * An XAPI client establishes a TLS connection to an XAPI service. | ||
| This case is outside the scope of trusted certificates managed by XAPI and is included here only for completeness. | ||
| This case is outside the scope of trusted certificates managed by XAPI, except when the client authenticates with a TLS client certificate (see [Client certificate authentication](#client-certificate-authentication)). |
There was a problem hiding this comment.
Add a point for the new client certificate usecasee instead of modifying an existing one.
| For example, if "Peer Bundle" doesn't exist and the "CA Bundle" (if not empty) is selected to do the validation, the endpoint should not try with "General Bundle" even when the validation with "CA Bundle" failed. | ||
|
|
||
| ## Client certificate authentication | ||
| When client certificate authentication is enabled with "pool.enable_client_certificate_auth", stunnel on the pool coordinator verifies the certificate chain of a TLS client certificate before the connection reaches XAPI without further authentication. |
There was a problem hiding this comment.
stunnel on the pool coordinator
Shouldn't stunnels on the other members also do the auth before telling the client to contact the coordinator? This seems quite narrow and could produce issues if HA is enabled and the coordinator needs to change.
There was a problem hiding this comment.
The "pool.enable_client_certificate_auth" only takes effect on the coordinator. On a member host, the cert based client auth would fail, then fallback to regular auth path. Then before applying the authenticating, the server would tell the client to contact the coordinator.
There was a problem hiding this comment.
This seems quite narrow and could produce issues if HA is enabled and the coordinator needs to change.
When a host becomes the coordinator, xapi restarts and sets up stunnel with its local copy of the bundle, so client certificate auth works on the new coordinator.
| ## Client certificate authentication | ||
| When client certificate authentication is enabled with "pool.enable_client_certificate_auth", stunnel on the pool coordinator verifies the certificate chain of a TLS client certificate before the connection reaches XAPI without further authentication. | ||
| The purpose "client_certificate_auth" marks the root CA certificates that are trusted for this. | ||
| It is only valid for root CA certificates ("ca" is true), because stunnel verifies the chain rather than pinning the certificate; a self-signed client certificate can be installed as its own root CA. |
There was a problem hiding this comment.
because stunnel verifies the chain rather than pinning the certificate; a self-signed client certificate can be installed as its own root CA.
I don't think this is true, this can be configured to it could change to expect leaf (pinned) certificates. I don't know if it's desireable or not.
There was a problem hiding this comment.
Generally it can. But in XAPI client authentication, it is required to use cert chain build + checkHost. So a user can install a pinned certificate, but it will never be used.
| It is only valid for root CA certificates ("ca" is true), because stunnel verifies the chain rather than pinning the certificate; a self-signed client certificate can be installed as its own root CA. | ||
|
|
||
| The coordinator combines two bundles into "/etc/stunnel/xapi-client-auth-ca-bundle.pem", which stunnel uses as its CA file: | ||
| * the "Default Bundle", so that CA certificates installed with "pool.install_ca_certificate" keep working until that API is removed; |
There was a problem hiding this comment.
Is there a plan to remove that API call? on XS 10.0?
There was a problem hiding this comment.
I think the "Default Bundle" should not be used here. It contains two kinds of trusted certificates:
- the ones for Xapi (as client with a client stunnel) to connect to other services (like WLB)
- the ones for Xapi (as server with a server stunnel) to accept Xapi clients.
The 2 is to be replaced by the new trusted certificate purpose. So the CAfile should point to the bundle file for the new purpose.
The 1 needs to be replaced by other new purpose, like appliance. Yes, this can be done on the next big version.
|
|
||
| Unlike the bundles for outgoing connections, these two are combined rather than chosen by precedence. | ||
| stunnel reads the combined bundle only when it starts or reloads, so the coordinator regenerates it whenever trusted certificates are installed or uninstalled, and reloads stunnel if it changed. | ||
| If neither bundle has a certificate, client certificate authentication is left out of the stunnel configuration, and XAPI authenticates every connection as usual. |
There was a problem hiding this comment.
and XAPI authenticates every connection as usual.
Remove this, it makes it look like xapi does not authenticate every connection normally when the bundles are present.
I mean Pool.install_ca_certificate. Besides XAPI's outgoing connections, stunnel also uses that CA bundle to verify client certificates once client cert auth is enabled. So a pool-operator can install their own CA, enable client cert auth, and log in as pool-admin. @Zhengchai |
Fair point. This is a draft to get early feedback on the approach, and testing on XS9 is still in progress. I'll rewrite the design section and commit messages before marking it ready for review. @psafont |
| * the "Default Bundle", so that CA certificates installed with "pool.install_ca_certificate" keep working until that API is removed; | ||
| * the "CA Bundle" of the purpose "client_certificate_auth". | ||
|
|
||
| Unlike the bundles for outgoing connections, these two are combined rather than chosen by precedence. |
There was a problem hiding this comment.
I think the purpose of the purpose is not to combine.
Installing trusted certificates is of the minimum role as pool operator. But passing certificate based client auth will gain pool-admin role. I think this needs a restriction on installing the trusted certificates with this purpose: only pool admin role can install trusted certificates with this purpose. |
|
I suggest putting the design and the UTs into separate commits. |
Add the use case of an XAPI client that logs in with a TLS client certificate, and the new purpose "client_certificate_auth" for it. stunnel on the coordinator uses the CA bundle of this purpose, and only pool-admin can install certificates for it. The "Default Bundle" is not used for client certificate auth any more. Signed-off-by: Yizhou Chen <yizhou.chen@citrix.com>
A session logged in with a TLS client certificate gets the "client-cert" role. This role has read-only access, plus the calls that are added one by one with "++ _R_CLIENT_CERT" in the datamodel. A client that manages a pool with a client certificate needs the same access as pool-admin. Give these sessions the permissions of pool-admin. Remove the client-cert role, the "++ _R_CLIENT_CERT" additions and the "++" operator, which was only used for them. Every call allowed for client-cert is also allowed for pool-admin, so no access is lost. The sessions still have is_local_superuser=false, so the calls in _R_LOCAL_ROOT_ONLY are still refused. Role UUIDs come from the role names, so the other roles don't change. The schema hash changes, because client-cert was in the reader and writer roles of many fields through _R_ALL. Signed-off-by: Yizhou Chen <yizhou.chen@citrix.com>
stunnel on the coordinator verifies client certificates with the bundle of the deprecated Pool.install_ca_certificate. This bundle also has the CAs for xapi's own connections to other services, such as WLB. stunnel only reads it when it starts or reloads, and installing or uninstalling a CA does neither. Add a new purpose "client_certificate_auth" and use its CA bundle as the CAfile of stunnel. CAs installed by Pool.install_ca_certificate are not used for client certificate login any more. The coordinator reloads stunnel when the bundle changes. If the bundle is empty, client certificate auth is not configured, as stunnel fails to start with a missing CAfile. Only root CAs can be installed for this purpose. A client certificate login has the pool-admin permissions, so installing certificates for this purpose needs a new permission that only pool-admin has. The new enum value changes the schema hash. Signed-off-by: Yizhou Chen <yizhou.chen@citrix.com>
Check that a client certificate session gets the permissions of pool-admin, that a pinned certificate can't be installed for client_certificate_auth, and that installing a certificate for it needs the new permission. Signed-off-by: Yizhou Chen <yizhou.chen@citrix.com>
9bac42c to
02c8d3a
Compare
|
|
||
| Only root CA certificates can be installed for this purpose, because stunnel is configured to verify the certificate chain. | ||
| A self-signed client certificate can be installed as a root CA. | ||
| A client certificate login has the permissions of pool-admin, so only pool-admin can install certificates for this purpose. |
There was a problem hiding this comment.
Add reason here why we will remove client auth in this feature, so that we will change the role of this into pool admin.
| , "Trusted certificates that are for LDAPS connections to external \ | ||
| authentication services." | ||
| ) | ||
| ; ( "client_certificate_auth" |
There was a problem hiding this comment.
The context is already for trusted certificate, so it can be client_auth without any ambiguity.
|
@psafont Hi, I have updated the design and it has been reviewed by local Toolstack. Could you please review the design again? Appreciate it! |
| if List.mem `client_certificate_auth purpose then ( | ||
| (* stunnel checks client certificates by chain verification, not pinning *) | ||
| if not ca then | ||
| raise Api_errors.(Server_error (invalid_value, ["ca"; "false"])) ; |
There was a problem hiding this comment.
The invalid_value is still ambiguous.
| let pool_cert = !Xapi_globs.server_cert_internal_path | ||
|
|
||
| let client_auth_bundle = | ||
| Printf.sprintf "%s/%s-client_certificate_auth.pem" |
There was a problem hiding this comment.
This can be derived from purpose instead of hard-coding.
| match (client_auth_name, client_auth_cas) with | ||
| | Some _, None -> | ||
| (* stunnel fails to start with a missing CAfile *) | ||
| warn "No CA is trusted for client certificate auth" ; |
There was a problem hiding this comment.
This is an error. I think we can add a gating in pool.enable_client_certificate_auth: to enable the client certificate auth, the CA must not be empty.
| in | ||
| let client_auth_cas = | ||
| Option.bind client_auth_name (fun _ -> | ||
| try Some (Unixext.string_of_file client_auth_bundle) |
There was a problem hiding this comment.
May the file exist but be empty? In any case, the stunnel service should fall back to client auth disabled state so that the XenAPI can work still.
Explain in the design why client certificate logins get the permissions of pool-admin instead of the "client-cert" role. Signed-off-by: Yizhou Chen <yizhou.chen@citrix.com>
|
enable_client_certificate_auth still only needs _R_POOL_OP. A pool-operator who holds any certificate from that CA can set name to their own CN/SAN and log in as pool-admin. |
last-genius
left a comment
There was a problem hiding this comment.
Comments by @minglumlu and @BengangY need to be addressed. Looks good to me otherwise.
| It also checks that the name in the client certificate matches "pool.client_certificate_auth_name". | ||
| If the check passes, the client can log in without a password. Otherwise, the regular authentication is used. | ||
|
|
||
| The client certificate is only checked on the coordinator. A pool member tells the client to contact the coordinator. |
There was a problem hiding this comment.
I would reword this as:
| The client certificate is only checked on the coordinator. A pool member tells the client to contact the coordinator. | |
| A valid client certificate is able to login only to a pool coordinator. When trying to use it on pool members, they send a response to contact the coordinator instead. |
| If the check passes, the client can log in without a password. Otherwise, the regular authentication is used. | ||
|
|
||
| The client certificate is only checked on the coordinator. A pool member tells the client to contact the coordinator. | ||
| The trusted certificates are copied to all hosts in the pool, so a new coordinator has the same bundle when the coordinator is changed. |
There was a problem hiding this comment.
| The trusted certificates are copied to all hosts in the pool, so a new coordinator has the same bundle when the coordinator is changed. | |
| The trusted certificates are copied to all hosts in the pool, so a new coordinator has the updated bundle when the coordinator is changed. |
| A self-signed client certificate can be installed as a root CA. | ||
|
|
||
| Client certificate logins used to get the "client-cert" role. | ||
| This role is removed, because a client that manages a pool this way needs the same access as pool-admin. |
There was a problem hiding this comment.
It's not clear what needs this, I'd prefer it if it was reworded:
| This role is removed, because a client that manages a pool this way needs the same access as pool-admin. | |
| This role is removed, because a client that manages a pool is considered to have the pool-admin role. |
Client certificate sessions get the pool-admin permissions, and the CAs for client certificate auth are installed with a new trusted certificate purpose.
make test, make check and the quality gate pass.