rpc: Support protocol version negotiation #371

ueno · 2021-06-04T09:30:16Z

This works as follows:

a couple of build-time constants have been added to represent the
minimal and maximum supported protocol versions
the client sends the maximum supported version upon the connection
establishment (when C_Initialize is called for the first time for the client process)
the server checks the version sent from the client; if it is lower
than the minimum supported version of the server, sends an error
otherwise, the server responds with either of smaller value between
the version sent from the client or the maximum supported version of
the server

Example:

the client supports versions from 1 upto 3
the server supports versions from 0 upto 2
the client sends 3 to the server upon connection establishment
the server responds with 2
version 2 is negotiated

Partially fixes #370

Signed-off-by: Daiki Ueno <ueno@gnu.org>

coveralls · 2021-06-04T09:33:01Z

Coverage decreased (-0.01%) to 71.854% when pulling 52e0194 on ueno:wip/dueno/version-negotiation into 576bba3 on p11-glue:master.

ueno · 2021-06-04T09:33:15Z

It's a shame that this doesn't accompany any automated tests (because it would requires multiple versions compiled in), but it could be tested manually by running:

$ P11_KIT_DEBUG=rpc _build/p11-kit/test-transport /transport/unix/basic
...
(p11-kit:19432) rpc_C_Initialize: authenticated with protocol version 1

after modifying P11_RPC_PROTOCOL_VERSION_MAXIMUM.

ep69

I am a bit confused to be honest, mostly because this is more like ground work if I get it correctly. Is it true that this is more like anticipating a future where protocol will have to change?

Further concerns:

What should happen if client version min > server version max?
What about testing? If protocol really changes in future, we need automated tests. Perhaps adding some override for strict checking of mins and maxes that can be specified at runtime?

configure.ac

ep69 · 2021-06-08T09:49:52Z

p11-kit/rpc-server.c

 	case 0:
 		goto out;
 	case 1:
-		if (version != 0) {
+#if P11_RPC_PROTOCOL_VERSION_MINIMUM > 0


I don't understand this #if. Isn't 0 the current default of P11_RPC_PROTOCOL_VERSION_MINIMUM? Or does P11_RPC_PROTOCOL_VERSION_MINIMUM == 0 mean "don't bother with checking versions"?

This is to avoid static analyzer warning: otherwise the following if condition always holds.

ep69 · 2021-06-08T09:54:22Z

p11-kit/rpc-transport.c

@@ -642,6 +619,51 @@ rpc_transport_uninit (p11_rpc_transport *rpc)
 	p11_buffer_uninit (&rpc->options);
 }

+static CK_RV
+rpc_transport_authenticate (p11_rpc_client_vtable *vtable,


Is rpc_transport_authenticate a reasonable name? We are negotiation protocol version rather than authenticating, aren't we?

This is named after AUTH command in D-Bus, and actually it allows the peers to exchange Unix credentials (UID and GID), so I think it's kind of authentication :-)

I see the analogy neither in D-Bus specification, nor in RPC. But I am no expert, I'll believe you know what you are doing.

In the D-Bus specification, more relevant section is Authentication Protocol, where not only SASL-based authentication but also capability negotiations like Unix FD passing are part of the protocol, though there is no version negotiation (I guess it's not needed at this level of the protocol, because usually the D-Bus interface version is maintained at the interface level). But anyway, I don't have a strong opinion on the naming of the method.

Me neither, feel free to keep it. I was just slightly confused.

ep69 · 2021-06-08T10:16:05Z

p11-kit/rpc-client.c

@@ -755,10 +756,17 @@ rpc_C_Initialize (CK_X_FUNCTION_LIST *self,
 	assert (module->vtable->connect != NULL);
 	ret = (module->vtable->connect) (module->vtable, reserved);

+	if (ret == CKR_OK) {


Shouldn't be client further initialized to send P11_RPC_PROTOCOL_VERSION_MAXIMUM?

It's not possible to send data, if connect returns error.

I might just be confused here. My concern was, the protocol description says client should tell server maximum version it supports. I don't see this logic anywhere in the code, so I'm assuming it sends 0 because of some implicit zeroing of some structure. Am I wrong?

It's sent in the default implementation of the authenticate method (rpc_transport_authenticate):
https://github.com/p11-glue/p11-kit/pull/371/files#diff-71598b22704d395f7e1134bcb2149f62123d7617365db84f3146cc5842115fe0R644

Ah, then I missed it, thanks for pointing it out.

ueno · 2021-06-08T18:19:18Z

Yes, this is a groundwork for future changes; we've already faced situations where we realized issues cannot be fixed without breaking the current protocol: #275 and #366 are the examples.

What should happen if client version min > server version max?

Good point; if there is no overwrap, the client should close the connection.

What about testing? If protocol really changes in future, we need automated tests. Perhaps adding some override for strict checking of mins and maxes that can be specified at runtime?

Indeed, I think the server side test case could be easily extended for that, as it crafts RPC messages, but the client side needs some mechanism to exercise the scenario.

ep69

Followed up in some comments.

About the high level concerns, I still don't see the logic for the case where client_version_max < server_version_min. Also, no tests appear for the version negotiation.

Anyway, I'll mark this approved not to block you further in case I am totally wrong.

ep69 · 2021-06-18T12:53:56Z

p11-kit/rpc-client.c

@@ -755,10 +756,17 @@ rpc_C_Initialize (CK_X_FUNCTION_LIST *self,
 	assert (module->vtable->connect != NULL);
 	ret = (module->vtable->connect) (module->vtable, reserved);

+	if (ret == CKR_OK) {


I might just be confused here. My concern was, the protocol description says client should tell server maximum version it supports. I don't see this logic anywhere in the code, so I'm assuming it sends 0 because of some implicit zeroing of some structure. Am I wrong?

ep69 · 2021-06-18T12:55:56Z

p11-kit/rpc-transport.c

@@ -642,6 +619,51 @@ rpc_transport_uninit (p11_rpc_transport *rpc)
 	p11_buffer_uninit (&rpc->options);
 }

+static CK_RV
+rpc_transport_authenticate (p11_rpc_client_vtable *vtable,


I see the analogy neither in D-Bus specification, nor in RPC. But I am no expert, I'll believe you know what you are doing.

ep69 · 2021-06-21T10:35:46Z

One more high-level thought: Let's say there is indeed a problem in future that requires a change of the protocol. Is this groundwork really all we can do at this moment? Perhaps there is something else that would help in future that could be done already (still thinking about figuring out a reasonable way to test protocol changes).

This works as follows: - a couple of build-time constants have been added to represent the minimal and maximum supported protocol versions - the client sends the maximum supported version upon connection establishment (when C_Initialize is called for first time from the client process) - the server checks the version sent from the client; if it is lower than the minimum supported version of the server, sends an error - otherwise, the server responds with either of smaller value between the version sent from the client and the maximum supported version of the server Signed-off-by: Daiki Ueno <ueno@gnu.org>

ueno · 2021-07-09T05:25:39Z

I agree, though we probably wouldn't be able to identify what else is missing without a version bump. Let's explore in that area in the follow-up PRs.

rpc: Separate authentication step from transaction

f583bd9

Signed-off-by: Daiki Ueno <ueno@gnu.org>

ueno force-pushed the wip/dueno/version-negotiation branch 5 times, most recently from 8fc5d3e to 330db46 Compare June 4, 2021 14:44

ep69 suggested changes Jun 8, 2021

View reviewed changes

ueno force-pushed the wip/dueno/version-negotiation branch from 330db46 to 5edfec6 Compare June 10, 2021 09:26

ep69 approved these changes Jun 18, 2021

View reviewed changes

ueno force-pushed the wip/dueno/version-negotiation branch from 5edfec6 to 52e0194 Compare July 9, 2021 05:17

ueno merged commit 4769c47 into p11-glue:master Jul 9, 2021

ueno added this to the 0.24.1 milestone Jan 17, 2022

ueno mentioned this pull request Jan 27, 2022

p11-kit-client.so RPC protocol and other servers #411

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: Support protocol version negotiation #371

rpc: Support protocol version negotiation #371

ueno commented Jun 4, 2021 •

edited

Loading

coveralls commented Jun 4, 2021 •

edited

Loading

ueno commented Jun 4, 2021 •

edited

Loading

ep69 left a comment

ep69 Jun 8, 2021

ueno Jun 10, 2021

ep69 Jun 8, 2021

ueno Jun 10, 2021

ep69 Jun 18, 2021

ueno Jun 19, 2021

ep69 Jun 21, 2021

ep69 Jun 8, 2021

ueno Jun 15, 2021

ep69 Jun 18, 2021

ueno Jun 19, 2021

ep69 Jun 21, 2021

ueno commented Jun 8, 2021

ep69 left a comment

ep69 Jun 18, 2021

ep69 Jun 18, 2021

ep69 commented Jun 21, 2021

ueno commented Jul 9, 2021

rpc: Support protocol version negotiation #371

rpc: Support protocol version negotiation #371

Conversation

ueno commented Jun 4, 2021 • edited Loading

coveralls commented Jun 4, 2021 • edited Loading

ueno commented Jun 4, 2021 • edited Loading

ep69 left a 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

ueno commented Jun 8, 2021

ep69 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ep69 commented Jun 21, 2021

ueno commented Jul 9, 2021

ueno commented Jun 4, 2021 •

edited

Loading

coveralls commented Jun 4, 2021 •

edited

Loading

ueno commented Jun 4, 2021 •

edited

Loading