Added changes for handling password string for UserIdentityToken and …

[jchirantan]

…for private key in case of certificate based connections to prevent it's trace in memory

[CLAassistant]

All committers have signed the CLA.

[jchirantan]

Hi @jouniaro When I use CryptoUtil.base64decode, I can see occurrences of Password String in memory dump. Can you suggest some other option?

[pradeipk]

Hi @jouniaro, Any update on above query. Also is there any plan to migrate to higher version of JDK?

[jouniaro]

You should probably add an overload to base64Decode that takes char[] instead of String.

The stack is not under active development anymore and therefore there is no plan to update to higher JDK.

[jchirantan]

Hi @jouniaro I have made the changes in the PR as suggested by you. Please review the changed PR...

[jchirantan]

Hi @jouniaro , Any updates on the PR?

[jouniaro]

Looks better. I am still concerned of this part: "If the user has passed password in plain text, then decoding will result in exception" - in case the user's password can be decoded, but it shouldn't, how do we enable that?

Thinking about this a bit more, base64 encoding the password should probably be handled outside 'loadFromKeyStore' altogether. It's not a security measure, just obscuring the password a bit, so I don't see it relevant to be automatically supported inside this method.

So, in the end, you would just like to add an overload to 'loadFromKeyStore' that takes the password as 'char[]' instead of String. This would enable handling the password as 'char[]' right from the start. And the original could just convert the String to 'char[]' and call it.

You can keep the base64Decode with 'char[]' anyway.

[jouniaro]

Actually, the base64Decode that takes the 'char[]' still creates a String from of it. I think you would like to prevent that as well.

[jchirantan]

Hi @jouniaro, Thanks for your suggestion. Wanted to answer some queries raised by you:

• To answer the first query:

  • User will pass actual password(Plaintext).
  • Client application does the job of encoding password. Later this will be decoded when it is passed to OPC stack as parameter.
  • Just for backward compatibility, If client application has password as a plaintext it is handled via try catch mechanism

• To answer the second query regarding password encoding:

  • Base64 encoding is handled outside "loadFromKeyStore" method. Client Application should pass Base64 encoded password String as parameter in this method.
  • So, only decoding is done inside this function.

• Also, can you suggest how we can prevent creation of String from char[] in Base64.decode() method which is inside bouncycastle library?

[jchirantan]

Hi @jouniaro, any updates?

[jouniaro]

Base64 encoding is not secure encoding and OPC UA does not specify it to be used for passwords. OPC UA specifies secure encoding of passwords, if a proper security policy is defined. So we don't want to add any non-standard Base64 decoding in there either. Therefore, if you want to use Base64 for handling passwords internally in your client or server application, it is better to do it outside the stack, but the stack should only handle plain text and then use the defined encryption policies.

If you wish avoid Strings with Base64 encoded, you should use the 'byte[]' version of the APIs. If you wish to add support for that to the stack, you would need to add the respective overload to CryptoProvider, CertificateProvider and CertificateUtils, which all have the base64Decode method (for certain reasons). The implementation for the BC and SC versions (BcCryptoProvider, ScCryptoProvider, BcCertificateProvider, ScCertificateProvider), would look like this:

@Override
public byte[] base64Decode(byte[] bytes) {
    return Base64.decode(bytes);
}

As to how to convert char[] to byte[], refer to this StackOverflow question, for example.

[jchirantan]

Hi @jouniaro,

• I have removed Base64 encoding and decoding part from code as per your suggestions
• Also, introduced overloaded methods to accept password in character array format
• I have kept overloaded methods for base64Decode() to pass character array as input instead of String, in CryptoProvider, CertificateProvider, CertificateUtils, BcCryptoProvider, ScCryptoProvider, BcCertificateProvider, ScCertificateProvider(Whichever classes have base64Decode method and the methods which call base64Decode method) as mentioned by you, for future reference.
• Please review the PR

[jouniaro]

Yeah, looks better in general. Would be better to make the 'string' versions of the functions call the 'char []' versions, to avoid two complete copies of the same functions, though. Could you do that as well?

[jchirantan]

Hi @jouniaro, Thanks for your reply. I have made the changes suggested by you. Can you please review the PR now?

[jchirantan]

Hi @jouniaro, Any updates on PR?

[jchirantan]

Hi @jouniaro, Any Updates on PR?

[jouniaro]

I reviewed the code and made still some changes to clarify it. And it didn't compile :)

Please, verify my commit yourself and create a new PR, if OK for you.

[jchirantan]

Hi @jouniaro, I will implement the changes suggested by you and will raise another PR. Closing this PR now