Support AES-GCM encryption algorithms#129
Conversation
mitchellhenke
force-pushed
the
mitchellhenke/support-gcm-encryption
branch
from
bb0b3b1 to
2651d43
Compare
spec/lib/saml_idp/encryptor_spec.rb
Outdated
| encrypted_doc = Nokogiri::XML::Document.parse(encrypted_xml) | ||
| encrypted_data = Xmlenc::EncryptedData.new(encrypted_doc.at_xpath('//xenc:EncryptedData', | ||
| Xmlenc::NAMESPACES)) |
There was a problem hiding this comment.
is it worth doing an xml query for the specific xml namespace like http://www.w3.org/2009/xmlenc11#aes256-gcm from the other method what was edited?
There was a problem hiding this comment.
maybe a controller spec (like this one)to ensure that that encryption/decryption of responses works with different encryption algos?
There was a problem hiding this comment.
Added specs. It doesn't seem like tripledes-cbc works at all due to a potential issue in upstream xmlenc or how saml_idp integrates with it, so I've removed it.
lib/saml_idp/encryptor.rb
Outdated
| when 'aes256-gcm' | ||
| 'http://www.w3.org/2009/xmlenc11#aes256-gcm' | ||
| else | ||
| "http://www.w3.org/2009/xmlenc11##{block_encryption}" |
There was a problem hiding this comment.
should we have an else here? or should we throw if it's not an expected encryption algorithm? (the block_encryption value is derived from the service provider in the database)
There was a problem hiding this comment.
I think we definitely could, xmlenc will throw anyway:
Xmlenc::UnsupportedError - Unsupported encryption method http://www.w3.org/2001/04/xmlenc#abc-haha
There was a problem hiding this comment.
Changed to raise a ValidationError
Sgtpluck
left a comment
Sgtpluck
left a comment
There was a problem hiding this comment.
left a couple questions, worth nothing that actually wiring this up to be accessible to partners will require an update of the partner portal and the config script (which currently just ignores the attribute in the data transformation and hardcodes the block encryption to aes256-cbc if it's 'none')
we probably want to identify a default as well, since we want to discourage/disallow partners from turning it off in production. historically it's been aes256-cbc since that was the only value ... do we want to use aes256-gcm?
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
I'm trying to figure out if Salesforce supports GCM. If that's the case, I'd vote for defaulting to that, so we can get a whole bunch of SP's to add encryption. |
mitchellhenke
force-pushed
the
mitchellhenke/support-gcm-encryption
branch
from
d2baeec to
45352a0
Compare
4 participants
closes https://gitlab.login.gov/lg-teams/Melba/protocols-backlog/-/issues/139
xmlenc added support for AES-GCM in version 0.8.0 in August 2021. It doesn't appear that upstream saml_idp supports AES-GCM either.
This PR adds support for using AES-GCM in the
block_encryptionoption.I did some light testing locally with our sample SAML service provider, and it everything seemed to work as expected.