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

Expose a method for encrypting using a remote v2_key #13083

Merged

Conversation

carbonin
Copy link
Member

@carbonin carbonin commented Dec 9, 2016

These keys are saved as a part of configuring central admin.
When encrypted data must be send to a remote region, that data has to be encrypted using the remote region's encryption key.

This allows callers to encrypt the data so that the remote region can use it properly.

https://bugzilla.redhat.com/show_bug.cgi?id=1400995

/cc @bdunne @gmcculloug @gtanzillo

These keys are saved as a part of configuring central admin.
When encrypted data must be send to a remote region, that data
has to be encrypted using the remote region's encryption key.

This allows callers to encrypt the data so that the remote region
can use it properly.

https://bugzilla.redhat.com/show_bug.cgi?id=1400995
@carbonin carbonin force-pushed the allow_encrypting_using_remote_region_keys branch from 254392c to 511e666 Compare December 9, 2016 14:50
@carbonin
Copy link
Member Author

carbonin commented Dec 9, 2016

I'm still looking into what it would take to add a #decrypt

Right now only MiqPassword#encrypt takes a key file as a parameter, so I would have to explicitly set MiqPassowrd's @@key_root to get decrypt to work with an alternate key.

Alternatively, I could use the v2_key= method, but that says "used by tests only". How strict should be about following that. @kbrock what do you think?

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@miq-bot
Copy link
Member

miq-bot commented Dec 9, 2016

Checked commits carbonin/manageiq@511e666~...c81afae with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 🍪

@carbonin
Copy link
Member Author

carbonin commented Dec 9, 2016

Okay, so after a talk with @kbrock it looks like the current approach with setting the v2_key directly with MiqPassword.v2_key= causes a thread safety issue. Other things using MiqPassword.encrypt will end up using the v2_key we set rather than the one they expect.

I think the best route would be to punt on decrypt until we can make the required changes to MiqPassword so that we don't have to worry about the thread safety issues.

@carbonin carbonin force-pushed the allow_encrypting_using_remote_region_keys branch from c81afae to 511e666 Compare December 9, 2016 16:16
@carbonin carbonin changed the title [WIP] Expose a method for encrypting using a remote v2_key Expose a method for encrypting using a remote v2_key Dec 9, 2016
@carbonin carbonin removed the wip label Dec 9, 2016
@carbonin
Copy link
Member Author

carbonin commented Dec 9, 2016

Okay, @bdunne waiting on your review, then we can merge this and I'll pass the BZ to you.

@carbonin
Copy link
Member Author

ping @bdunne

I think we still need this, not sure where this is on your priorities though...

@bdunne bdunne merged commit 0f917f6 into ManageIQ:master Jan 12, 2017
@bdunne bdunne added this to the Sprint 52 Ending Jan 16, 2017 milestone Jan 12, 2017
@simaishi
Copy link
Contributor

@carbonin @bdunne I see the BZ is closed as not a bug. Does this still need to be backported to Euwe?

@carbonin
Copy link
Member Author

@simaishi Nope, I'll change the flag. This will actually likely be removed/refactored soon.

@carbonin carbonin added euwe/no and removed euwe/yes labels Jan 23, 2017
@carbonin carbonin deleted the allow_encrypting_using_remote_region_keys branch March 7, 2017 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants