For making ProtonSaslExternalImpl RFC compliant

[hariprasad-SAP]

Motivation:

RFC

SASL External mechanism is capable of transferring an authorization identity string. The client sends the initial response to the intial challenge by the server. It can be empty or non-empty.
Response is non-empty when the client is requesting to act as the identity represented by the (non-empty) string which is UTF-8 encoding of the requested authorization identity string. It is empty when the client is requesting to act as the identity the server associated with its authentication credentials.

We can notice that the initial response is configured in Line 26. It is always set to EMPTY (empty byte array defined here) and cannot be configured to a custom string that we can use as identity string. Hence this library is NOT RFC compliant

If we have to pass the authorization identity string to the server then we must configure the initial response of that client.

Conformance:

Your commits should be signed and you should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md
Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines

[vietj]

@gemmellr ping :-)

[gemmellr]

Most the same as my reply over at qpid-jms, main difference in the last sentence:

I wouldnt really say its 'not compliant' so much as it simply doenst support requesting to act as a particular non-server-associated identity, like various other clients (and servers) similarly dont support.

Which server is it you are looking to use that you actually require to pass a client-defined identity for EXTERNAL? I cant say I currently know of any that actually require it. Most I know of even ignore it, since its fairly typical for servers to want to govern the identity they consider the client to be, especially with EXTERNAL. Such reasons are why vertx-proton does not support this, it simply hasnt ever needed to, and there would actually be no point for various servers it has been common to use it with.

Its a notable change in behaviour to simply make it always do this now if the username field happens to be set, given it would have been ignored to this point. Perhaps an option would be needed to toggle the behaviour, though that is fairly ugly so I really dont want such a toggle..I guess I'd skip it for now and assess later.

It would be good if you can outline why you believe you need this currently. If then proceeding you would have to squash your commits, update the PR to be against main rather than 3.9 (it can be backported if it lands), and you also havent included any tests which which would be required.

[hariprasad-SAP]

Hi, Thank you for reviewing the PR and providing comments.

The use case is AMQP messaging with mTLS. At the client side, it will be having authorization identity string(username), Key and x509 certificates that is required to establish connection with Message Broker and do messaging.

Below is Vertx Proton Usage where we think to pass the identity-string as username

ProtonClient protonClient = ProtonClient.create(vertx);
ProtonClientOptions protonClientOptions = new ProtonClientOptions();
String PRIVATE_KEY = "-----BEGIN RSA PRIVATE KEY-----...-----END RSA PRIVATE KEY-----\n";
String CERTIFICATE = "-----BEGIN CERTIFICATE-----....-----END CERTIFICATE-----\n";

protonClientOptions.setKeyCertOptions(new PemKeyCertOptions()
				.setKeyValue(Buffer.buffer(PRIVATE_KEY))
				.setCertValue(Buffer.buffer(CERTIFICATE)))
				.setTrustAll(true);
protonClientOptions.addEnabledSaslMechanism("EXTERNAL");
protonClientOptions.setSsl(true);

// identity-string can be used as username, password can be null then
protonClient.connect(protonClientOptions, host, port, username, password, connAr -> {
    if (connAr.succeeded()) {
        connAr.result()
            .openHandler(h -> {
                if (h.succeeded()) {
                    // Connection Opened

                    protonConnection = h.result();
                    protonConnection.createSender(address)
                        .openHandler(oh -> {
                            if (oh.succeeded()) {
                                // sender opened

                                protonSender = oh.result();
                                
                                Message message = Message.Factory.create();
                                message.setBody(new Data(new Binary(
                                    "HelloWorld".getBytes(StandardCharsets.UTF_8))));

                                sender.send(message, delivery -> {
                                    log.debug("message sent");
                                    delivery.settle();
                                });
                            } else {
                                log.debug("failed to open sender");
                                promise.fail("err");
                            }
                        }).open();

                } else {
                    log.error("failed to open connection", h.cause());
                }
            }).open();

    } else {
        log.error("failed to connect", connAr.cause());
    }
});

We currently use 3.9 version of Vertx Proton and if this change can be backported from master, i will do the necessary changes to the PR with all review comments taken care and raise the PR against master branch.

Please review and let me know if this change proposal is acceptable or if any other ways you know of achieving mTLS with Vertx proton will be greatly appreciated.

[gemmellr]

You have failed to really answer my question. I understand that the use case is mTLS. You can already do this with vertx-proton (with or without the EXTERNAL SASL mech being used) essentially as your code sample does. It simply wont send the optional client-defined authorization identity, instead an indication for server-associated identity. (Aside: you obviously wouldnt want to use .setTrustAll(true) in a production use case as your code sample does, making it vulnerable to MITM).

I wondered which server you are looking to use it with that you believe you actually need this change, i.e that server requires specifying the optional client-defined authorization identity, rather than using the server-associated one coming from its own trust of the clients certificate when it was originally allowed to actually connect the transport connection before SASL occurs.

You do not need to specify a client-defined authorization identity in any cases I know of currently. Many servers I know of also wont use a client-defined value even if present. Thats why vertx-proton and qpid-jms dont do it, they havent ever needed to and it would typically be pointless even if they did.