Skip to content
This repository has been archived by the owner on Dec 2, 2022. It is now read-only.

Support different keys for decryption and signature verification. #15

Merged
merged 6 commits into from
Sep 2, 2019
Merged

Conversation

deskoh
Copy link
Contributor

@deskoh deskoh commented Aug 31, 2019

Client can only be configured to use the same private key (i.e. key.pem) for signature verification and assertion decryption.

This PR allows different keys to be configured.

@LoneRifle
Copy link
Contributor

Under what circumstances is there a need to support different keys? From what I understand, spcp setups typically recycle the decrypt assertion key for signing. Also.. is there a way to make this change without breaking backward compatibility?

@deskoh
Copy link
Contributor Author

deskoh commented Aug 31, 2019

Correct me if I'm wrong, my understanding is that the application keys (or more specifically the X.509 certs) used by SPCP for signature (verification) and assertion encryption are provided by the service provider, both of which could (and should) be different?

@LoneRifle
Copy link
Contributor

LoneRifle commented Sep 2, 2019

Your understanding is correct - that said, I feel the need to retain backward compatibility so that those who have opted to use the same certs can continue to specify it as appKey in settings, perhaps as a fallback for this.appSigningKey and this.appEncryptionKey, ie, in the ctor,

this.appSigningKey = config.appSigningKey || config.appKey
this.appEncryptionKey = config.appEncryptionKey || config.appKey

The PR lgtm otherwise. If you could incorporate my proposed change I'll happily accept then kick out a minor release

@deskoh
Copy link
Contributor Author

deskoh commented Sep 2, 2019

Good suggestion and valid point.

To simplify usage, I added optional appEncryptionKey to be provided if it is different from appKey. This will avoid the need to deal with up to 3 'keys' parameters in the config, (Furthermore, the appKey is also used for JWT signing, which will make the usage even more complicated).

Aside, is it desirable to create a TypeScript definition? Included some jsdoc type error fixes in this PR as well.

@LoneRifle
Copy link
Contributor

I would if I could. Happy to take a separate PR for that

Copy link
Contributor

@LoneRifle LoneRifle left a comment

Choose a reason for hiding this comment

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

lgtm. thanks for incorporating my suggestions

@LoneRifle LoneRifle merged commit 82c5392 into opengovsg:master Sep 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants