Skip to content

Add pool.sync_trusted_certificates_from API#7056

Merged
minglumlu merged 3 commits into
xapi-project:feature/ldapsfrom
minglumlu:private/mingl/CP-311981
May 13, 2026
Merged

Add pool.sync_trusted_certificates_from API#7056
minglumlu merged 3 commits into
xapi-project:feature/ldapsfrom
minglumlu:private/mingl/CP-311981

Conversation

@minglumlu

@minglumlu minglumlu commented May 7, 2026

Copy link
Copy Markdown
Member

Add a new API pool.sync_trusted_certificates_from to facilitate the secure LDAP feature in pool.join case. It is also a general API to download trusted certificates.

Comment on lines 271 to 275
Note over client,coor: sync all ldaps certs
client->>coor: pool.download_trusted_certificate
client->>coor: pool.download_trusted_certificates
coor-->>client:
client->>join: pool.install_trusted_certificate
join-->>client:

@psafont psafont May 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the client need to be involved in the certificate exchange between the hosts? Can't the joiner join the domain as part of the pool join, after the certificate exchange is done there?

Regarding the API call, would it be possible to request certificates for a singular purpose, instead of a list of IDs? It's less flexible this way, but harder to mis-use, IMO

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed when joining a host (does not enabled AD) to a pool with AD enabled (and the pool has ldaps enabled). In this case XC will ask user to join AD and continue the pool join.
Here XC will call the download and upload certs API to sync ldaps certs between the joining host and the pool.

I'm also not convinced by this implementation, the client does not have access to the purpose for each certificate with the current API. This is needed to be able to install the certificates for the right uses
Does the list API contain the purpose details?

My question is, do we prefer download_trusted_certificates other than download_trusted_certificate, in later case, user can freely download a cert and save into a file. (and does not need to manage the certs order)

@minglumlu minglumlu May 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The client can get the purpose (a set) from the certificate record separately. An API like this is flexible for the programming with SDK which doesn't need to save into a file at all. A client can download one with this API as well.

Downloading all certificates for a singular purpose is possible but it would cause error when the exchanging happens during pool.join. The exchanging will fail if the purpose sets don't match.

Alternatively, we could return the purpose set also. But this would make the return data structure complicated.

Why does the client need to be involved in the certificate exchange between the hosts? Can't the joiner join the domain as part of the pool join, after the certificate exchange is done there?

This was discussed in the design stage. The trusted certificates are necessary to ensure the secure LDAP work well before pool.join, although they would be exchanged automatically during pool.join.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Downloading all certificates for a singular purpose is possible but it would cause error when the exchanging happens during pool.join. The exchanging will fail if the purpose sets don't match.

I'm not sure I understand this point, I'm imagining the actions of the client like this:

client-->coor: pool.download_trusted_certificates ~purpose:"ldaps"
for certificate in certificates:
client-->join: pool.install_trusted_certificate ... ~purpose:"ldaps"

After this, the pool join will find that the certificates for the purpose have to be exchanged (the UUIDs) do not match, but I don't see how it would make the join to fail.

This also means that the call that's being propose wouldn't deduplicate certificates, and it's another why the certificate exchange and LDAP joining should happen at pool join, where all these things can be coordinated, while on the client they cannot.

@liulinC liulinC May 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The major reason why client is involved here is because the certificate sync is relatively late, https://github.com/xapi-project/xen-api/blob/master/ocaml/xapi/xapi_pool.ml#L1929
later than various pre_join_checks, specially assert_external_auth_matches.
The certs needs to be synced (at least for the purpose of ldaps) to fix this check to ensure pool homogeneous.
If the backend sync the certs, doesn't it make xapi too complicated and a little strange?
(Or we move the AD check right after xapi certificate sync? The side effect is the certificate is synced even pool join failed due to AD check. If this can be accepted, I am happy to update design to simplify XC)

For the duplicate certificates, can xapi filter the duplicates certs by fingerprint during certificate sync?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the new function I was thinking of adding it to ocaml/xapi/xapi_pool.ml using functions in
ocaml/xapi/cert_distrib.mli, similar to how exchange_trusted_certificates works, but only with ldaps certificates, and only with the download from the joined pool to the joiner host, withou without uploading certs to the joined pool.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. You prefer to get the client involved as little as possible. So ideally a single API would import ldaps certificates from the pool into the joining host and complete the check (and would rollback the importing if the checking failed).
While the current PR, in reverse, is offloading more logic to clients.

Actually I think your idea of "pool.download_trusted_certificates ~purpose:"ldaps" is fine if we could accept the certificates are absent for other purpose during pool.join. As I think this would work in most cases. And users can still resolve the issue if the certificates for other purpose are necessary during pool.join by installing the certificates with multiple purposes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @psafont
May I please know more about your thoughts on this?
The current proposal is pool.download_trusted_certificates ~purpose:"ldaps". And the client needs to install the dowloaded certs into the joining host. This would ignore the multi-purpose certificates case. In other words, the client needs to handle it in other way, like install the certificates with multiple purposes before pool.join.

Another point to be clear is that I think it would be better to keep the downloading and installing separated two APIs. They are for different pools. Combining them into one would have to pass one's credential to another.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current proposal is pool.download_trusted_certificates ~purpose:"ldaps". And the client needs to install the dowloaded certs into the joining host. This would ignore the multi-purpose certificates case. In other words, the client needs to handle it in other way, like install the certificates with multiple purposes before pool.join.

If the call with a purpose parameter is not working, the one you proposed for several certificates at the same time is fine.

I would like to nudge you again towards a solution where the client is not active participant in the transfer. The joining host controlling the transfer and installation of LDAPS certificates means that it should be able to retain all the metadata without much issues.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I force pushed two commits to introduce a new pool.sync_trusted_certificates_from to let the joining host do the job which is basically similar to what the pool.exchange_trusted_certificates does except that it is an uni-direction sync, from pool to joining host only.
I changed the name from download to sync to get it more accurate. And beside the remote pool's hostname or address, the new API pool.sync_trusted_certificates_from requires a remote_session to authenticate with the remote pool, and the pool coordinator's host certificate for verifying the identity of the remote pool.

So from the change of the document, now the client is not active participant of the transferring and installing. Most of the work are offloaded to the new API on the joining host.

minglumlu added 2 commits May 12, 2026 15:27
Signed-off-by: Ming Lu <ming.lu@cloud.com>
Signed-off-by: Ming Lu <ming.lu@cloud.com>
@minglumlu minglumlu force-pushed the private/mingl/CP-311981 branch from df52c2d to 91596ab Compare May 12, 2026 07:44
@minglumlu minglumlu changed the title Add pool.download_trusted_certificates API Add pool.sync_trusted_certificates_from API May 12, 2026
)
; (Bool, "ca", "true for 'ca' or false for 'pinned'")
]
~allowed_roles:(_R_POOL_OP ++ _R_CLIENT_CERT)

@psafont psafont May 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the call should return the certificates (references) that have been added the the system. This allows the client to delete them if the pool join fails, which the design document requires.

Signed-off-by: Ming Lu <ming.lu@cloud.com>
@minglumlu

minglumlu commented May 13, 2026

Copy link
Copy Markdown
Member Author
  • Complete all manual tests before merge.

@minglumlu minglumlu merged commit 651b8ba into xapi-project:feature/ldaps May 13, 2026
16 checks passed
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