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

Log encryption failures #127

Merged
merged 2 commits into from Dec 11, 2017
Merged

Conversation

mkanoor
Copy link
Contributor

@mkanoor mkanoor commented Dec 6, 2017

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

Customers might export/import models between different environments.
There typically is a different key for each environment. If the export
deck has encrypted fields, they are imported into the new environment
unchanged. This field cannot be decrypted at runtime in the Automate
engine, because of key mismatch.
During $evm.instantiate we were not logging any error messages and
customer has a tough time debugging the issue.

This PR logs the encryption error.

@mkanoor
Copy link
Contributor Author

mkanoor commented Dec 6, 2017

@gmcculloug @tinaafitz
Please review

Copy link
Member

@tinaafitz tinaafitz left a comment

Choose a reason for hiding this comment

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

@mkanoor Looks good.

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

Customers might export/import models between different environments.
There typically is a different key for each environment. If the export
deck has encrypted fields, they are imported into the new environment
unchanged. This field cannot be decrypted at runtime in the Automate
engine, because of key mismatch.
During $evm.instantiate we were not logging any error messages and
customer has a tough time debugging the issue.

This PR logs the encryption error.
@tinaafitz
Copy link
Member

@gmcculloug Please review.

MiqAePassword.new(MiqAePassword.decrypt(value))
rescue MiqPassword::MiqPasswordError => err
$miq_ae_logger.error("Error decrypting password #{err.message}. Is this password imported from a different environment?")
raise err
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about changing the log message to:
"Error decrypting password: [#{err.message}]. Possible cause: Password value was encrypted with a different encryption key."

The logging message from the included test would read:
Error decrypting password: [can not decrypt v0_key encrypted string]. Possible cause: Password value was encrypted with a different encryption key.

Minor but you can just use raise to re-raise the last error. You do not need to pass the object.

@miq-bot
Copy link
Member

miq-bot commented Dec 8, 2017

Checked commits mkanoor/manageiq-automation_engine@f51eb01~...3bc9a53 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 2 offenses detected

lib/miq_automation_engine/engine/miq_ae_engine/miq_ae_object.rb

lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service.rb

@mkanoor
Copy link
Contributor Author

mkanoor commented Dec 8, 2017

@gmcculloug
Please review, the red error is coming from code climate which complains about too many returns which has existed in the code before. I dont want to refactor that for now.

@gmcculloug
Copy link
Member

Agreed, that it does not need to be refactored in this PR.

@gmcculloug gmcculloug merged commit bcc9490 into ManageIQ:master Dec 11, 2017
@gmcculloug gmcculloug added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 11, 2017
@gmcculloug gmcculloug self-assigned this Dec 11, 2017
simaishi pushed a commit that referenced this pull request Dec 11, 2017
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit b15999b9fb8d7b13d990bff4f1e7aace84a47b7e
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Mon Dec 11 09:04:21 2017 -0500

    Merge pull request #127 from mkanoor/log_decryption_error
    
    Log encryption failures
    (cherry picked from commit bcc9490a9053328d4808befb6bd79473f7aa8755)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1524617

@simaishi
Copy link
Contributor

Fine backport (to manageiq repo) details:

$ git log -1
commit 83d93203ec0a13a49f65c5e1fe67ab6bf322731d
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Mon Dec 11 09:04:21 2017 -0500

    Merge pull request #127 from mkanoor/log_decryption_error
    
    Log encryption failures
    (cherry picked from commit bcc9490a9053328d4808befb6bd79473f7aa8755)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1525551

@simaishi
Copy link
Contributor

Euwe backport (to manageiq repo) details:

$ git log -1
commit 9cf6de416a2f6f06e8572ed778f01a64f3d2ad71
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Mon Dec 11 09:04:21 2017 -0500

    Merge pull request #127 from mkanoor/log_decryption_error
    
    Log encryption failures
    (cherry picked from commit bcc9490a9053328d4808befb6bd79473f7aa8755)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1549629

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.

None yet

5 participants