core/comms/secure_channel: Always set remote nonce

[laumann]

The set_remote_nonce_from_byte_string() performs some checks against the message security mode.

In our tests, we use security policy = None and security mode = None, but the server sets Basic126Rsa15 for the user identity token policy meaning that we still need to use the server nonce for authentication.

This patch removes all the checking around security policy and mode, which allows the client to connect to our OPC-UA server (Kepware) successfully.

[laumann]

@locka99 This is the other part of the #58 - I'm less confident that this is the right change to make (and it breaks a test) than in #112. All I know at this point is that if security policy = None and message security policy = None, but user identity token policy != None (so something like Basic128Rsa15) we need the server nonce at least for encrypting the password during authentication.

[svanharmelen]

I just encountered this same issue (also when connecting to, an somewhat older, Kepware OPC-US server) and this patch fixed it for me as well 👍🏻

[laumann]

I just encountered this same issue (also when connecting to, an somewhat older, Kepware OPC-US server) and this patch fixed it for me as well 👍🏻

Thanks for the feedback! With regards to storing the nonce, there are some notes in the spec (I don't remember where) on when the nonce should be stored. In my use case, I definitely need the nonce for the client to encrypt the password for authentication, but the connection being opened doesn't itself do encryption nor message signing (so at that point we don't need the nonce).

So I'm wondering needs to be adjusted so the client stores the nonce if necessary for authentication and then deletes it when it knows that it won't be needed again.

[locka99]

If we can get it passing and not obviously breaking anything with no crypto then I'll take it

[laumann]

If we can get it passing and not obviously breaking anything with no crypto then I'll take it

OK, I'll see what I can do.

[svanharmelen]

For completeness, this was the error that I got yesterday:

[ERROR opcua_core::comms::secure_channel] Remote nonce is invalid length 32, expecting 16. [125, 7, 118, 64, 63, 160, 205, 183, 159, 119, 133, 225, 198, 45, 133, 15, 242, 79, 117, 180, 48, 6, 72, 99, 26, 131, 5, 127, 18, 158, 39, 110]

[svanharmelen]

@locka99 I've looked into the failing tests hoping to get it sorted so this could be merged, but I don't think these tests can be fixed... With the changes in this PR, I'd say we should just remove this test. What do you think?

#[test]
pub fn secure_channel_nonce() {
    let mut sc = SecureChannel::new_no_certificate_store();
    sc.set_security_mode(MessageSecurityMode::SignAndEncrypt);
    sc.set_security_policy(SecurityPolicy::Basic256);
    // Nonce which is not 32 bytes long is an error
    assert!(sc
        .set_remote_nonce_from_byte_string(&ByteString::null())
        .is_err());
    assert!(sc
        .set_remote_nonce_from_byte_string(&ByteString::from(b""))
        .is_err());
    assert!(sc
        .set_remote_nonce_from_byte_string(&ByteString::from(b"1"))
        .is_err());
    assert!(sc
        .set_remote_nonce_from_byte_string(&ByteString::from(b"0123456789012345678901234567890"))
        .is_err());
    assert!(sc
        .set_remote_nonce_from_byte_string(&ByteString::from(
            b"012345678901234567890123456789012".as_ref()
        ))
        .is_err());
    // Nonce which is 32 bytes long is good
    assert!(sc
        .set_remote_nonce_from_byte_string(&ByteString::from(b"01234567890123456789012345678901"))
        .is_ok());
}

[svanharmelen]

@locka99 any suggestion on how to handle this test? Would love to get this in master so I don't have to keep working from a local branch 😉 Thanks!

[locka99]

I'll try and look at this later this week. I haven't had much time to work on the repo recently

[svanharmelen]

Cool... Thanks for the update/info 👍🏻

[laumann]

Thanks for following up on this, I'd like to get it merged as well. I'm still not sure the implementation is the right one though. There are some requirements on nonces IIRC, but I cannot recall what they are at this moment (don't have the spec handy).

[locka99]

I have the patch applied and I can see the failing test. I need to find time this weekend to sit down with the spec and see if I can decipher exactly when it should be there, what length it should be and if there are side effects of supplying it even with a security profile of None when I can't infer the length.

[locka99]

I've checked in a version that allows the nonce to be set when security policy is None. I have preserved the length validation for other policies. I don't know if this is right, but it allows you to set the nonce to anything you like. The documentation isn't very helpful and only says the certificate and nonce are optional when security policy is none but doesn't say what they should contain.

[svanharmelen]

Thanks @locka99! I'll test this out this week 👍🏻