Add certificate password provider interface to support password protected pfx files. #1291
Conversation
mregen
commented
Feb 18, 2021
•
mregen
commented
- add CertificatePasswordProvider to SecurityConfiguration, if set, it is used when a private key is loaded
- on purpose password cannot be set in config files, ICertificatePasswordProvider must be implemented to allow custom implementation of secure storage of the private key password
- sample in ref server how to use it.
- implementation in ApplicationInstance/Config to avoid breaking changes.
Stack/Opc.Ua.Core/Security/Certificates/CertificateIdentifier.cs
Outdated
Show resolved
Hide resolved
| s_exitCode = ExitCode.ErrorServerNotStarted; | ||
| ConsoleSampleServer().Wait(); | ||
| ExitCode = ExitCode.ErrorServerNotStarted; | ||
| await ConsoleSampleServer().ConfigureAwait(false); |
There was a problem hiding this comment.
Async method should end with "Async". Also, name does not indicate what method does. Suggestion: StartSampleConsoleServerAsync
There was a problem hiding this comment.
StartConsoleReferenceServerAsync ...
| @@ -294,18 +305,18 @@ private void PrintSessionStatus(Session session, string reason, bool lastContact | |||
| lock (session.DiagnosticsLock) | |||
| { | |||
| StringBuilder item = new StringBuilder(); | |||
| @@ -177,7 +182,7 @@ public async Task Start(ServerBase server) | |||
|
|
|||
| if (m_applicationConfiguration == null) | |||
| { | |||
| await LoadApplicationConfiguration(false); | |||
| await LoadApplicationConfiguration(false).ConfigureAwait(false); | |||
There was a problem hiding this comment.
Method name should end with "Async"
There was a problem hiding this comment.
the async suffix is not enforced in this codebase (yet). Also for back compat such a API change needs to be carefully considered. There is are some improvements for async in the pipeline, when they are ready it might be a good time to also apply breaking async name changes to other API..
| @@ -266,7 +274,9 @@ public void Stop() | |||
| /// </summary> | |||
| public async Task<ApplicationConfiguration> LoadApplicationConfiguration(string filePath, bool silent) | |||
| { | |||
| ApplicationConfiguration configuration = await LoadAppConfig(silent, filePath, ApplicationType, ConfigurationType, true); | |||
| ApplicationConfiguration configuration = await LoadAppConfig( | |||
| if (m_applicationConfiguration == null) | ||
| { | ||
| await LoadApplicationConfiguration(silent); | ||
| await LoadApplicationConfiguration(silent).ConfigureAwait(false); |
There was a problem hiding this comment.
Method name should end with Async. Also, consider changing name to explain difference between this and method "LoadAppConfig" or create an overload.
There was a problem hiding this comment.
someday hopefully LoadApplication can be improved when there are more breaking changes...
|
|
||
| // check that it is ok. | ||
| if (certificate != null) | ||
| { | ||
| certificateValid = await CheckApplicationInstanceCertificate(configuration, certificate, silent, minimumKeySize); | ||
| certificateValid = await CheckApplicationInstanceCertificate(configuration, certificate, silent, minimumKeySize).ConfigureAwait(false); |
There was a problem hiding this comment.
that would be a breaking API change
| @@ -2155,7 +2155,8 @@ public ServerStatusDataType GetStatus() | |||
| /// <returns>Boolean value.</returns> | |||
| public async Task<bool> RegisterWithDiscoveryServer() | |||
| { | |||
| ApplicationConfiguration configuration = string.IsNullOrEmpty(base.Configuration.SourceFilePath) ? base.Configuration : await ApplicationConfiguration.Load(new FileInfo(base.Configuration.SourceFilePath), ApplicationType.Server, null, false); | |||
| ApplicationConfiguration configuration = string.IsNullOrEmpty(base.Configuration.SourceFilePath) ? | |||
| base.Configuration : await ApplicationConfiguration.Load(new FileInfo(base.Configuration.SourceFilePath), ApplicationType.Server, null, false); | |||
There was a problem hiding this comment.
Consider adding parameter names to null and false
There was a problem hiding this comment.
again just code style... better in a seperate PR.
| @@ -3260,7 +3261,7 @@ public UserIdentityToken UserIdentity | |||
| /// The reverse connect information. | |||
| /// </summary> | |||
| [DataMember(Name = "ReverseConnect", Order = 8, IsRequired = false)] | |||
| public ReverseConnectEndpoint ReverseConnect | |||
| public ReverseConnectEndpoint ReverseConnect | |||
There was a problem hiding this comment.
Why not use auto-properties for all properties here? shorter:
public ReverseConnectEndpoint ReverseConnect { get; set; }
There was a problem hiding this comment.
thats not of concern in this PR
| @@ -529,13 +536,13 @@ private static bool IsValidCertificateBlob(byte[] rawData) | |||
| // extract length. | |||
| length = rawData[2]; | |||
|
|
|||
| for (int ii = 0; ii < lengthBytes-1; ii++) | |||
| for (int ii = 0; ii < lengthBytes - 1; ii++) | |||
There was a problem hiding this comment.
Consider using a more meaningful name instead of ii, for example: offset
|
Hi @luiscantero, all good feedback but this PR is not the right place for style or API breaking changes. We should track it and do some seperate PR for styling and async improvements.. |
