CA-398341: Populate fingerprints of CA certificates on startup #6006

snwoods · 2024-09-18T16:07:33Z

Also CP-51527: Add --force option to pool-uninstall-ca-certificate.

Addresses the issues raised here by @stormi #5955

snwoods · 2024-09-18T16:17:46Z

ocaml/xapi-cli-server/cli_frontend.ml

@@ -378,8 +378,10 @@ let rec cmdtable_data : (string * cmd_spec) list =
  ; ( "pool-certificate-uninstall"
    , {
        reqd= ["name"]
-      ; optn= []
-      ; help= "Uninstall a pool-wide TLS CA certificate."
+      ; optn= ["force"]


pool-certificate-uninstall is deprecated, so should I be updating it or leaving it as-is? My initial instinct was the latter, but they both use the same underlying function Cli_operations.pool_uninstall_ca_certificate and so the --force option functionality is included in pool-certificate-uninstall regardless.

Please leave it as-is, same as the api call

I've removed force from both this and certificate_uninstall in datamodel_pool.ml, but when testing pool-certificate-uninstall, force still works. This is expected because as I say, certificate-uninstall is using Cli_operations.pool_uninstall_ca_certificate, the same as pool-uninstall-ca-certificate. Is this what you intended? It adds functionality to the deprecated function but doesn't change the documentation.

(datamodel_pool.certificate_uninstall is completed unused AFAICT)

snwoods · 2024-09-18T16:20:49Z

ocaml/xapi-cli-server/cli_frontend.ml

+      ; optn= ["force"]
+      ; help= "Uninstall a pool-wide TLS CA certificate. The optional parameter \
+               '--force' will remove the DB entry even if the certificate file is \
+               non-existent"
      ; implementation= No_fd Cli_operations.pool_uninstall_ca_certificate
      ; flags= [Deprecated ["Use pool-uninstall-ca-certificate"]]


What does this Deprecated flag actually do? It doesn't show when you do xe help pool-certificate-uninstall and it didn't show when I ran the command itself.

This is probably because there's very little autogenerated code. The cli server should let the client print a message, probably. (will it break the internal regression tests, though?)

snwoods · 2024-09-18T20:28:51Z

ocaml/xapi/xapi_pool.ml

@@ -1397,9 +1397,9 @@ let certificate_install ~__context ~name ~cert =

 let install_ca_certificate = certificate_install

-let certificate_uninstall ~__context ~name =


Trying to understand how the legacy names work. Why is the legacy name "certificate_uninstall" still needed (here and in message_forwarding.ml and datamodel_pool.ml if the deprecated xe command doesn't even use it? It calls pool_install_ca_certificate which calls uninstall_ca_certificate, not certificate_uninstall. What is it providing compatibility with, is this for upgrades from much older xapis?

API clients might still use this function, this is why it needs to be kept.

gangj · 2024-09-19T08:39:03Z

ocaml/xapi/certificates.ml

+            ( Eq (Field "fingerprint_sha256", Literal "")
+            , Eq (Field "fingerprint_sha1", Literal "")
+            )
+        , Eq (Field "type", Literal "ca")


May I know do we also need to upgrade for the "host" certificate, thank you~

Yes we do, but I added this already in certificates_sync.sync (https://github.com/xapi-project/xen-api/blob/master/ocaml/xapi/certificates_sync.ml#L96-L104). This PR is because I originally missed updating the user/ca certificates too 😅

OK, I see, thank you!
Then can we also cover the "host" certificate upgrade here and then we don't need the same functionality in two places.

Yeah, I wondered about that, but certificates_sync.sync already existed (it does more than just update the fingerprint_sha* fields) so it makes sense for host certificates to be updated there. However, it currently only updates host certs. Would it make sense to expand certificates_sync.sync to include user certificates too so that we have it in one place? What do you think @psafont ?

I don't think it makes sense as it is, currently. It's used for host certificates, exclusively, when the before the stunnel component gets started.

These certificates gets used by the client part, so the update needs to happen at a time that's independent (or risk introducing races) Maybe we can consolidate some of the modules, because the certificate handling is a bit disperse now.

So, leave it as it is in this PR for now but with the potential to consolidate the certificate handling code in the future?

That's right

psafont · 2024-09-19T11:13:21Z

ocaml/xapi/certificates.ml

+            let sha256 = pp_fingerprint ~hash_type:`SHA256 certificate in
+            Ok (sha1, sha256)
+          with Unix.Unix_error (Unix.ENOENT, _, _) ->
+            Error (`Msg (Printf.sprintf "filename %s does not exist" filename))


Can you add another catch for any exception? This is out of cautiosness. Because the function is called on startup, it cannot raise any exception.

| exn -> Error (`Msg (Printexc.to_string exn))

psafont · 2024-09-19T11:14:39Z

ocaml/idl/datamodel_pool.ml

+        (String, "name", "The certificate name")
+      ; ( Bool
+        , "force"
+        , "If true, remove the DB entry even if the file is non-existent"


This call is deprecated, please don't modify it. (we have to change the lifecycle, but the last entry is the deprecation!)

psafont · 2024-09-19T11:14:59Z

ocaml/idl/datamodel_host.ml

+      ; ( Bool
+        , "force"
+        , "If true, remove the DB entry even if the file is non-existent"
+        )
      ]
    ~allowed_roles:_R_LOCAL_ROOT_ONLY
    ~lifecycle:[(Published, "1.290.0", "Uninstall TLS CA certificate")]


Please add an entry to the lifecycle stating it has been modified

psafont · 2024-09-19T11:15:43Z

ocaml/idl/datamodel_pool.ml

+        , "force"
+        , "If true, remove the DB entry even if the file is non-existent"
+        )
+      ]
    ~allowed_roles:(_R_POOL_OP ++ _R_CLIENT_CERT)
    ~lifecycle:[(Published, "1.290.0", "Uninstall TLS CA certificate")]


Please add an entry to the lifecycle, as in the other call

ocaml/xapi/message_forwarding.ml

psafont · 2024-09-23T09:35:40Z

ocaml/xapi/certificates.ml

@@ -342,17 +345,21 @@ let host_install kind ~name ~cert =
      (ExnHelper.string_of_exn e) ;
    raise_library_corrupt ()

-let host_uninstall kind ~name =
+let host_uninstall ?(force = false) kind ~name =


Suggested change

let host_uninstall ?(force = false) kind ~name =

let host_uninstall kind ~name ~force =

There's no need to make the parameter optional. Optional parameters complicate usage in some occasions, because of their interaction with currying; and they are really only useful in situations where compatibility needs to be maintained, or other contrived situations.

In cases where we control all the users, such as this one, it's better to use named arguments

That's fair, I think I made it optional to reduce how many places were changed and because the cli call is optional and I didn't initially realise that it defaults to false when the option is not supplied. But that makes sense!

psafont · 2024-09-23T09:36:39Z

ocaml/xapi/certificates.ml

@@ -448,8 +456,9 @@ let pool_install kind ~__context ~name ~cert =
    ) ;
    raise exn

-let pool_uninstall kind ~__context ~name =
-  host_uninstall kind ~name ; pool_sync ~__context
+let pool_uninstall ?(force = false) kind ~__context ~name =


Suggested change

let pool_uninstall ?(force = false) kind ~__context ~name =

let pool_uninstall kind ~__context ~name ~force =

ocaml/xapi/xapi_pool.ml

snwoods · 2024-09-23T10:50:13Z

ocaml/idl/datamodel_host.ml

+      [
+        (Published, "1.290.0", "Uninstall TLS CA certificate")
+      ; ( Changed
+        , "24.29.0"


@psafont Is this the correct thing to do? This is currently the correct version but it could change before release. Does this just need to be updated again before release or is there some way to auto-fill it? This only happens when you have lifecycle:[] right?

These versions are painful to encode because they cannot be calculated atomatically, like the case of lyfecycle:[]. Right now, this should be 24.31.0 (because 24.30.0 has been released)

robhoes · 2024-10-01T09:51:48Z

ocaml/xapi/xapi.ml

@@ -1147,6 +1147,10 @@ let server_init () =
            , []
            , fun () -> report_tls_verification ~__context
            )
+          ; ( "Update shared certificate's metadata"
+            , [Startup.OnlyMaster]
+            , fun () -> Certificates.Db_util.upgrade_ca_fingerprints ~__context


Is this a one-off upgrade case? That is, once you have the xapi version with this change in it and it has run once, you'll never need it anymore? Then it would better fit in xapi_db_upgrade.ml (bump the schema version and add a < rule).

robhoes · 2024-10-01T10:00:22Z

ocaml/idl/datamodel_host.ml

-        (Ref _host, "host", "The host"); (String, "name", "The certificate name")
+        (Ref _host, "host", "The host")
+      ; (String, "name", "The certificate name")
+      ; ( Bool


I think this would break existing clients, because the new param does not have a default. Use ~versioned_params for that.

robhoes

See above.

lindig · 2024-10-04T10:42:33Z

Can we pick this up again? @snwoods

robhoes · 2024-10-08T12:17:33Z

ocaml/idl/datamodel_host.ml

+          param_type= Bool
+        ; param_name= "force"
+        ; param_doc= "Remove the DB entry even if the file is non-existent"
+        ; param_release= numbered_release "24.31.0"


Please make it 24.32.0, as we have 31 already released.

robhoes · 2024-10-08T12:18:49Z

ocaml/xapi/xapi_db_upgrade.ml

+let upgrade_ca_fingerprints =
+  {
+    description= "Upgrade the fingerprint fields for ca certificates"
+  ; version= (fun x -> x < (5, 779))


Why 779? You have used 783 in the datamodel.

Ah 779 was when the fingerprint_sha256 and fingerprint_sha1 fields were added, so I was thinking anything from that version should already have those fields populated, but of course if they'd previously upgraded from <779 they wouldn't have it, sorry.

robhoes · 2024-10-08T12:24:10Z

ocaml/xapi/xapi_db_upgrade.ml

@@ -904,6 +904,66 @@ let upgrade_update_guidance =
      )
  }

+let upgrade_ca_fingerprints =


This is dead code until you add the new function to the rules list below.

SHA256 and SHA1 certificates' fingerprints do not get populated when the database is upgraded, so empty values need to be detected and amended on startup. Signed-off-by: Pau Ruiz Safont <[email protected]> Signed-off-by: Steven Woods <[email protected]>

This allows the CA certificate to be removed from the DB even if the certificate file does not exist. Signed-off-by: Steven Woods <[email protected]>

psafont

This contains an unrelated commit from #6045

let's wait until that PR is merged

snwoods · 2024-10-08T15:27:28Z

Tested on a toolstack-next host and it worked as expected (updated the fingerprints when updating to this code, but didn't run the upgrade code from then on)

psafont · 2024-10-08T15:50:59Z

Please remove my commit and rebase on top of master, then it should be good to go

snwoods commented Sep 18, 2024

View reviewed changes

snwoods force-pushed the private/stevenwo/CA-398341 branch from 7104aae to ad30824 Compare September 18, 2024 16:33

snwoods commented Sep 18, 2024

View reviewed changes

snwoods mentioned this pull request Sep 18, 2024

fingerprint_sha256 and fingerprint_sha1 empty after upgrade for user certs in /etc/stunnel/certs/ #5955

Open

gangj reviewed Sep 19, 2024

View reviewed changes

psafont reviewed Sep 19, 2024

View reviewed changes

ocaml/xapi/message_forwarding.ml Show resolved Hide resolved

snwoods force-pushed the private/stevenwo/CA-398341 branch from ad30824 to 12e4c1d Compare September 23, 2024 09:26

psafont reviewed Sep 23, 2024

View reviewed changes

snwoods commented Sep 23, 2024

View reviewed changes

ocaml/xapi/xapi_pool.ml Show resolved Hide resolved

snwoods force-pushed the private/stevenwo/CA-398341 branch from 12e4c1d to ea59ccf Compare September 23, 2024 09:51

snwoods commented Sep 23, 2024

View reviewed changes

snwoods force-pushed the private/stevenwo/CA-398341 branch from ea59ccf to 8512a2e Compare September 23, 2024 16:54

psafont approved these changes Sep 24, 2024

View reviewed changes

robhoes reviewed Oct 1, 2024

View reviewed changes

robhoes requested changes Oct 1, 2024

View reviewed changes

snwoods force-pushed the private/stevenwo/CA-398341 branch 2 times, most recently from 69cfd55 to a210e11 Compare October 8, 2024 11:07

robhoes reviewed Oct 8, 2024

View reviewed changes

psafont and others added 2 commits October 8, 2024 14:06

CP-51527: Add --force option to pool-uninstall-ca-certificate

df94a76

This allows the CA certificate to be removed from the DB even if the certificate file does not exist. Signed-off-by: Steven Woods <[email protected]>

snwoods force-pushed the private/stevenwo/CA-398341 branch from a210e11 to df94a76 Compare October 8, 2024 13:07

robhoes approved these changes Oct 8, 2024

View reviewed changes

psafont requested changes Oct 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CA-398341: Populate fingerprints of CA certificates on startup #6006

CA-398341: Populate fingerprints of CA certificates on startup #6006

snwoods commented Sep 18, 2024

snwoods Sep 18, 2024

psafont Sep 19, 2024

snwoods Sep 23, 2024

snwoods Sep 18, 2024

psafont Sep 19, 2024

snwoods Sep 18, 2024

psafont Sep 24, 2024

gangj Sep 19, 2024 •

edited

Loading

snwoods Sep 19, 2024

gangj Sep 19, 2024

snwoods Sep 23, 2024

psafont Sep 23, 2024

snwoods Sep 23, 2024

psafont Sep 24, 2024

psafont Sep 19, 2024

psafont Sep 19, 2024

psafont Sep 19, 2024

psafont Sep 19, 2024

psafont Sep 23, 2024

snwoods Sep 23, 2024

psafont Sep 23, 2024

snwoods Sep 23, 2024

psafont Sep 23, 2024

robhoes Oct 1, 2024

robhoes Oct 1, 2024

robhoes left a comment

lindig commented Oct 4, 2024

robhoes Oct 8, 2024

robhoes Oct 8, 2024

snwoods Oct 8, 2024

robhoes Oct 8, 2024

psafont left a comment

snwoods commented Oct 8, 2024

psafont commented Oct 8, 2024

		@@ -1397,9 +1397,9 @@ let certificate_install ~__context ~name ~cert =

		let install_ca_certificate = certificate_install

		let certificate_uninstall ~__context ~name =

	let host_uninstall ?(force = false) kind ~name =
	let host_uninstall kind ~name ~force =

	let pool_uninstall ?(force = false) kind ~__context ~name =
	let pool_uninstall kind ~__context ~name ~force =

CA-398341: Populate fingerprints of CA certificates on startup #6006

Are you sure you want to change the base?

CA-398341: Populate fingerprints of CA certificates on startup #6006

Conversation

snwoods commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gangj Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robhoes left a comment

Choose a reason for hiding this comment

lindig commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psafont left a comment

Choose a reason for hiding this comment

snwoods commented Oct 8, 2024

psafont commented Oct 8, 2024

gangj Sep 19, 2024 •

edited

Loading