CP-41028: Enforce certificate checking for intra-pool storage migration connections #4845
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
Currently, certificate verification is hardcoded off inside this helper function, which is used mostly in storage migration. So far, this parameter has been effectively unused, since the relevant code path use plain HTTP connections. Now that we will start using HTTPS everywhere, verification needs to be controlled and explicitly configured higher up. The value of verify_cert has not yet been modified, so there is no functional change yet. Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
The code that constructs rpc functions has been moved to Storage_utils and extended with options to turn on certificate checking of the remote server. The following commits will set those parameters to the right values depending on whether the storage migration takes place within a pool, or cross pool. Also removed the use of the term "local" around this code, as in some cases these weren't actually used for host-local connections. Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
…A.copy(_into) This option controls whether or not to verify the certificate of the remote server. This is enabled only for calls (as part of storage migration) within a pool at the moment. Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
When starting a storage copy or mirror operation, the call now comes with a verify_dest parameter, indicating whether the remote server's certificate must be verified when connecting to it. These calls return an ID for the operation, which is performed asynchronously. Any following calls regarding the same operation, e.g. to stop or cancel, only take this ID as an argument, but may require new connections to the remote. For this reason, a state record with connection details is maintained for every operation. This commit adds the verify_dest parameter to this state, and to all the places in calls that use the state to make connections. Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
Similar to DATA.MIRROR.start and DATA.copy(_into). Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
This is used to export VM metadata to another host during live migration. Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
lindig
approved these changes
Nov 14, 2022
| @@ -339,7 +318,10 @@ let vdi_info x = | |||
| failwith "Runtime type error: expecting Vdi_info" | |||
|
|
|||
| module Local = StorageAPI (Idl.Exn.GenClient (struct | |||
| let rpc call = rpc ~srcstr:"smapiv2" ~dststr:"smapiv2" (local_url ()) call | |||
| let rpc call = | |||
| Storage_utils.rpc ~srcstr:"smapiv2" ~dststr:"smapiv2" | |||
There was a problem hiding this comment.
Should we have a module-local binding for the smapiv2 string and use that everywhere? That would eliminate any possibility to spell it wrong.
There was a problem hiding this comment.
This is only for logging, and not used very consistently. I think we should sort this out properly when we get down to do proper distributed tracing. This stuff may then become obsolete.
| let module Remote = StorageAPI (Idl.Exn.GenClient (struct | ||
| let rpc = | ||
| rpc ~srcstr:"smapiv2" ~dststr:"dst_smapiv2" (storage_url remote_url) | ||
| Storage_utils.rpc ~srcstr:"smapiv2" ~dststr:"dst_smapiv2" remote_url |
There was a problem hiding this comment.
Is the dst_ prefix correct?
psafont
reviewed
Nov 14, 2022
ocaml/xenopsd/lib/xenops_server.ml
Outdated
| @@ -2408,7 +2408,7 @@ and perform_exn ?subtask ?result (op : operation) (t : Xenops_task.task_handle) | |||
| in | |||
| debug "%s compress memory: %b" __FUNCTION__ compress_memory ; | |||
| let verify_cert = | |||
| if vmm.vmm_verify_cert then Stunnel_client.pool () else None | |||
| if vmm.vmm_verify_cert then Some Stunnel.pool else None | |||
There was a problem hiding this comment.
This is confusing, but it's exactly what the doctor ordered
There was a problem hiding this comment.
maybe a comment explaining the situation would help?
psafont
approved these changes
Nov 14, 2022
...to decide whether to use certificate checking or not. This module uses a local ref to determine whether checking is enabled on the host. In xapi, this ref is set from a file on startup, and after that it is updated if checking is enabled/disabled through the API. Xenopsd currently does not initialise the ref, so checking is always off. We could initialise the ref on startup, like in xapi, but then xenopsd still won't get updates if the host-level switch is flipped. Instead, let xapi worry about the host-level state, by querying Stunnel_client, and tell xenopsd exactly what to do. Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
…an values We use `verify_dest` to simply indicate whether to verify certicates or not. The name `verify_cert` is used for the more complex type that includes the CA path and SNI, as defined in the Stunnel module. Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.