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.

Sign up for GitHub

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

Jump to bottom

Enable custom signer integrations #296

Closed
wants to merge 2 commits into from
Closed

Enable custom signer integrations #296

wants to merge 2 commits into from

Conversation

ksmithRenweb
Copy link

These changes allow us to use Azure KeyVault's Signer with Mimekit.

Key Vault stores all of our signature keys. To use them for signing, we have to let Azure KeyVault do the signing.

So to use Mimekit to create well formatted emails with attachments, and to use Azure KeyVault do the signing, slight modifications were needed.

@jstedfast
Copy link
Owner

What if I add a public virtual ISigner DigestSigner { get; } property to DkimSigner and provide you with a protected .ctor that you could use from a custom subclass that also overrides the new property?

@jstedfast
Copy link
Owner

What properties could you provide to the .ctor? I take it you can't get access to the private key?

@ksmithRenweb
Copy link
Author

ksmithRenweb commented Apr 3, 2017 •
edited by jstedfast
Loading

This is the heart of what has to happen to use the azure signer.

protected virtual async Task<byte[]> GenerateSignatureForStream()
{
    var baseUri = _serviceConfiguration.GetConfigurationValue("KeyVault", "VaultBaseUri");

    if (!_forSigning)
        throw new InvalidOperationException("RsaDigestSigner not initialised for signature generation.");

    byte[] hash = new byte[_digest.GetDigestSize()];
    _digest.DoFinal(hash, 0);

    var whoknows = await _azureKeyVaultClient.SignAsync(baseUri, _keyName, _version, JsonWebKeySignatureAlgorithm.RS256,
        hash);

    return whoknows.Result;
}

An asynchronous web service call where we say use this keyvault address, with this key, of this version on this signature hash.

@ksmithRenweb
Copy link
Author

"What if I add a public virtual ISigner DigestSigner { get; } property to DkimSigner and provide you with a protected .ctor that you could use from a custom subclass that also overrides the new property?"

I could use that. If that is your preferred method.

@jstedfast
Copy link
Owner

I've added 2 new protected .ctors:

protected DkimSigner (string domain, string selector, DkimSignatureAlgorithm algorithm);
protected DkimSigner (string domain, string selector);

and I've also added that virtual property to get the ISigner:

public virtual ISigner DigestSigner { get; }

jstedfast added a commit that referenced this pull request Apr 3, 2017
@jstedfast
Added a virtual DigestSigner properto to DkimSigner
45bc9e1 
This allows developers to provide MimeMessage.DkimSign()
with a custom ISigner implementation.

This feature is needed to allow developers to services
such as Azure KeyVault which do not provide direct
access to private keys and instead require developers
to use their own APIs for signing.

Fixes issue #296
@jstedfast
Copy link
Owner

I've just merged my patch. Let me know if that won't work for you and we can figure something out so that you can get the interfaces you need.

FWIW, I'm hoping to make another release this weekend if everything goes according to plan.

@jstedfast jstedfast closed this Apr 3, 2017
@ksmithRenweb
Copy link
Author

Did you do a prerelease nuget package I can test?

@jstedfast
Copy link
Owner

I don't do prerelease nugets generally

@ksmithRenweb
Copy link
Author

ksmithRenweb commented Apr 4, 2017 •
edited
Loading

It appears to be working. I'll let you know if we run into any problems.

A new production nuget package this weekend would be great.

@jstedfast
Copy link
Owner

Awesome. I'll try to make a new release this weekend.

@jstedfast
Copy link
Owner

@ksmithRenweb

I'm working on trying to optimize DKIM signing by trying to use System.Secrity.Cryptography instead of BouncyCastle for the backend and I ended up taking advantage of this feature myself :)

Question for you: Since RSACryptoServiceProvider needs to be disposed, it made me wonder if perhaps I could make MimeKit's DkimSignatureStream (which is what uses the DkimSigner.DigestSigner property) dispose the signer after it is done with it by checking if it implements IDisposable.

Would that help you? Hurt you? Make no difference either way?

@ksmithRenweb
Copy link
Author

ksmithRenweb commented Jan 5, 2018 via email •
edited by jstedfast
Loading

@jstedfast
Copy link
Owner

Turns out using System.Security didn't make things any faster :(

I'm not going to be able to drop BouncyCastle as I need it for other things (like cross-platform support on Mac/Linux), OpenPGP, and loading private keys and such. But yea, I can understand the annoyance of pulling in such a massive library when you don't explicitly need it.

Mono's System.Security supports some of the basics like RSA, SHA1, SHA256, and X509Certificates (mostly) - but does not implement things like an X509Store nor a lot of other necessary bits :(

For DKIM itself, I would probably be able to get away w/o having to depend on BouncyCastle at all though. Maybe I can try to move things in that direction in the future and include DKIM support in MimeKitLite. That would require breaking API/ABI again tho.

I'll add that to my "Next time I'm going to break API/ABI anyway..." TODO list :)

@jstedfast
Copy link
Owner

@ksmithRenweb just a heads up, but I just landed a pretty big patch to MimeKit that changes the way DKIM works.

For now, I'm leaving in place the MimeMessage.Sign() DKIM methods, but I've marked them Obsolete.

The new methods are DkimSigner.Sign() and this is where some changes that affect you are as well...

Instead of having a DkimSigner.DigestDigner property, I've moved that logic into DkimSigner.CreateSigningContext () which is protected virtual.

This is all part of an effort to implement ARC support which is similar to DKIM.

When I make a release with these changes in it, it will come with a 2.2.0 version tag.

If you have any suggestions that might make it easier to integrate with Azure, let me know and I can look into it.

@ksmithRenweb
Copy link
Author

ksmithRenweb commented Jun 3, 2019 via email •
edited by jstedfast
Loading

@ksmithRenweb
Copy link
Author

ksmithRenweb commented Jun 12, 2019 via email

@jstedfast
Copy link
Owner

No problem ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ksmithRenweb @jstedfast