Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: handle extra padding if key length > 2048 #648

Merged

Conversation

Maddin-619
Copy link
Contributor

@Maddin-619 Maddin-619 commented Apr 20, 2023

Connecting to a opc ua server with a 4096 bit key length gave me an error. The server could not decode the OpenSecureChannelRequest because the ExtraPaddingSize byte was missing.

If the key length is greater than 2048 bits there should be an ExtraPaddingSize byte in the message footer:
UA Part 6: Mappings - 6.7.2.5 Message Footer

The PR should fix that problem while encoding and decoding messages.

I implemented the fix in accordance with the c reference implmentation (open62541)

@Maddin-619 Maddin-619 force-pushed the fix/open_secure_channel_key_gt_2048 branch from 6e170c1 to f02922d Compare April 20, 2023 10:03
Copy link
Member

@magiconair magiconair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Could you also please add a test for this? Then I'll merge it.

uasc/secure_channel_instance.go Outdated Show resolved Hide resolved
@Maddin-619
Copy link
Contributor Author

It's not clear for me how and where to write tests for the secure_channel_instance. There are only a few tests for the newRequestMessage method. To test signAndEncrypt and verifyAndDecrypt we need test fixtures for the message and the channelInstance with an EncryptionAlgorithm (with mocked encrypt, decrypt, signature and verify functions). That is quite a lot to set up before writing the test that I need a little help with.

@magiconair
Copy link
Member

Hmm, fair enough. Let me have a look

@magiconair
Copy link
Member

I don't think this is so bad since signAndEncrypt is mostly self-contained. You only need to configure a channel instance with the right algorithm and security mode and a bare-bones message for this to work. Sometimes I factor this kind of code out into a standlone function to make it more testable.

Maybe start with something like this (code does NOT compile :))

func TestSignAndEncrypt(t *testing.T) {
    tests := []struct{
        name string
        c *channelInstance
        m *Message
        b []byte
        want []byte
        err error
    }{
        {"none", 
            &channelInstance{
                sc: SecureChannel{cfg: Config{SecurityMode: None}}
            },
            ...
        }
     }


    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            signed, err := tt.c.signAndEncrypt(tt.m, tt.b)
            if got, want := err.String(), tt.err.String(); got != want {
                t.Fatalf("got error %v want %v", got, want)
            }

            if got, want := signed, tt.want; !bytes.Equal(got, want) {
                ...   
            }
        }
    }
}

// create some helper funcs to create ciphertext from plaintext
// to make testing easier

@magiconair
Copy link
Member

Also, EncryptionAlgorithm already has Encrypt and Decrypt methods so there is no mocking IMHO. Just test with some real algorithms.

@magiconair
Copy link
Member

If it makes it easier you could put the golden plaintext data into testdata folder and use embed to pull it in.

@magiconair
Copy link
Member

@Maddin-619 let me know if that works for you or if you need more help.

@Maddin-619
Copy link
Contributor Author

I have problems using the real algorithems, because the encryption always leads to a different ciphertext. So we can't assert with static bytes.
I see two possible solutions:

  1. using the verifyAndDecrypt for the assertion getting back the plaintext
  2. mock the encrypt method s.t. plaintext = ciphertext

In general it should be better to isolate the testee without the dependencies. So I would prefer solution 2.

@magiconair magiconair added this to the v0.4.1 milestone Jun 13, 2023
@magiconair magiconair force-pushed the fix/open_secure_channel_key_gt_2048 branch from 0a7a842 to b39884e Compare June 13, 2023 15:18
@magiconair magiconair merged commit 4061161 into gopcua:main Jun 13, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants