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

Use crypto.Signer instead of *rsa.PrivateKey #114

Closed
wants to merge 1 commit into from

Conversation

nicksnyder
Copy link

@nicksnyder nicksnyder commented Jul 25, 2023

X509KeyStore currently only supports RSA private keys due to the type used and was wondering if we could expand support to any crypto.Signer.

This is the minimal diff, but it is backward incompatible from an API perspective so merging would imply a v2. Alternatively, I could update the diff to introduce new types and maintain backward compatibility (but wanted to start a convo first before I went further with this).

@nicksnyder nicksnyder marked this pull request as ready for review July 25, 2023 22:43
@russellhaering
Copy link
Owner

Great idea - I've got my own use-case for this coming along shortly too.

I don't think this is worth cutting a v2 over, so let's try to keep it backwards compatible for now, but I'll also start putting together a list of things for a future v2. Seem reasonable?

@nicksnyder
Copy link
Author

Sounds good. I can update the diff to implement this in a backward compatible way.

@nicksnyder
Copy link
Author

nicksnyder commented Aug 2, 2023

As I looked more deeply into this, I discovered that #89 already introduced the capability to use a crypto.Signer. Given this capability already exists, it seems like it would be redundant to move forward with a change like this. Do you have a use case that is not supported by #89?

@nicksnyder
Copy link
Author

What I really wanted was crypto.Signer support in gosaml2, so I opened a PR there to use the existing capability in this library.

@nicksnyder nicksnyder closed this Aug 4, 2023
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.

2 participants