Skip to content
This repository was archived by the owner on Nov 21, 2018. It is now read-only.

Support more certificate loading scenarios (#69) #85

Merged
merged 5 commits into from
May 2, 2017

Conversation

cesarblum
Copy link
Contributor

#69

@dnfclas
Copy link

dnfclas commented Apr 28, 2017

@CesarBS,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@cesarblum cesarblum changed the base branch from dev to rel/2.0.0-preview1 April 28, 2017 21:30
#if NETCOREAPP2_0
TryLoad(X509KeyStorageFlags.EphemeralKeySet, out error) ??
#endif
TryLoad(X509KeyStorageFlags.UserKeySet, out error);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@blowdart @bartonjs Does this look right?

Actually, unless I'm missing something, the #if here doesn't make sense since we're targeting netstandard1.3 only. Should I only leave it as UserKeySet?

Copy link
Member

Choose a reason for hiding this comment

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

@Eilon @DamianEdwards @muratg @davidfowl
We should prefer ephemeral if it is available, which is only available in netcoreapp2.0. I believe that means this package (Microsoft.AspNetCore) needs to target netcoreapp2.0, not netstandard1.3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I put ephemeral first, I can't run the sample app. It loads the certificate but this happens on every request:

fail: Microsoft.AspNetCore.Server.Kestrel[0]
      Uncaught exception from the OnConnectionAsync method of an IConnectionAdapter.
System.ComponentModel.Win32Exception (0x80004005): No credentials are available in the security package
   at System.Net.SSPIWrapper.AcquireCredentialsHandle(SSPIInterface secModule, String package, CredentialUse intent, SCHANNEL_CRED scc)
   at System.Net.Security.SslStreamPal.AcquireCredentialsHandle(CredentialUse credUsage, SCHANNEL_CRED secureCredential)
   at System.Net.Security.SslStreamPal.AcquireCredentialsHandle(X509Certificate certificate, SslProtocols protocols, EncryptionPolicy policy, Boolean isServer)
   at System.Net.Security.SecureChannel.AcquireServerCredentials(Byte[]& thumbPrint)
   at System.Net.Security.SecureChannel.GenerateToken(Byte[] input, Int32 offset, Int32 count, Byte[]& output)
   at System.Net.Security.SecureChannel.NextMessage(Byte[] incoming, Int32 offset, Int32 count)
   at System.Net.Security.SslState.StartSendBlob(Byte[] incoming, Int32 count, AsyncProtocolRequest asyncRequest)
   at System.Net.Security.SslState.ProcessReceivedBlob(Byte[] buffer, Int32 count, AsyncProtocolRequest asyncRequest)
   at System.Net.Security.SslState.StartReadFrame(Byte[] buffer, Int32 readBytes, AsyncProtocolRequest asyncRequest)
   at System.Net.Security.SslState.StartReceiveBlob(Byte[] buffer, AsyncProtocolRequest asyncRequest)
   at System.Net.Security.SslState.ForceAuthentication(Boolean receiveFirst, Byte[] buffer, AsyncProtocolRequest asyncRequest)
   at System.Net.Security.SslState.ProcessAuthentication(LazyAsyncResult lazyResult)
   at System.Net.Security.SslStream.BeginAuthenticateAsServer(X509Certificate serverCertificate, Boolean clientCertificateRequired, SslProtocols enabledSslProtocols, Boolean checkCertificateRevocation, AsyncCallback asyncCallback, Object asyncState)
   at System.Net.Security.SslStream.<>c__DisplayClass35_0.<AuthenticateAsServerAsync>b__0(AsyncCallback callback, Object state)
   at System.Threading.Tasks.TaskFactory`1.FromAsyncImpl(Func`3 beginMethod, Func`2 endFunction, Action`1 endAction, Object state, TaskCreationOptions creationOptions)
   at System.Threading.Tasks.TaskFactory.FromAsync(Func`3 beginMethod, Action`1 endMethod, Object state)
   at System.Net.Security.SslStream.AuthenticateAsServerAsync(X509Certificate serverCertificate, Boolean clientCertificateRequired, SslProtocols enabledSslProtocols, Boolean checkCertificateRevocation)
   at Microsoft.AspNetCore.Server.Kestrel.Https.HttpsConnectionAdapter.<OnConnectionAsync>d__7.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.FrameConnection.<ApplyConnectionAdaptersAsync>d__32.MoveNext()

So I'm leaving the order as it is for now.

/// by name, or one or more inline certificate specifications.
/// </param>
/// <returns>One or more loaded certificates.</returns>
public IEnumerable<X509Certificate2> Load(IConfigurationSection certificateConfiguration)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danroth27 Do you want to keep the keys when loading from e.g.

"MyCert" : {
  "Source": "File",
  "Path": "mycert.pfx"
},
"MyCert2": {
  "Source": "Store",
  "StoreName": "My",
  "StoreLocation": "CurrentUser"
}

?

Copy link
Member

Choose a reason for hiding this comment

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

By "keys" do you mean the certificate name used in config? Yeah, I think it would be good to provide those.

Copy link
Member

Choose a reason for hiding this comment

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

But not required.


/// <summary>
/// Creates a new instance of <see cref="KestrelServerOptionsSetup"/>.
/// </summary>
Copy link

Choose a reason for hiding this comment

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

"Wrong!" :)


/// <summary>
/// Creates a new instance of <see cref="KestrelServerOptionsSetup"/> that can load certificates by name.
/// </summary>
Copy link

Choose a reason for hiding this comment

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

Ditto

@cesarblum
Copy link
Contributor Author

📢

Copy link

@muratg muratg left a comment

Choose a reason for hiding this comment

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

👍

@danroth27
Copy link
Member

We shouldn't throw if we look for certificates in store and don't find any. Instead we should throw where those certificates are being used (ex identity/kestrel).

@muratg
Copy link

muratg commented May 1, 2017

@CesarBS, @DamianEdwards thinks we should multi-target netstandard1.3 and netcoreapp2.0 so that we keep supporting desktop CLR apps.

@cesarblum cesarblum merged commit 605aedd into rel/2.0.0-preview1 May 2, 2017
@cesarblum cesarblum deleted the cesarbs/69 branch May 2, 2017 01:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants