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

Require a key attribute for the key_rsa resource #2891

Merged
merged 5 commits into from
Apr 12, 2018

Conversation

omar-irizarry
Copy link
Contributor

Checking that passphrase is not of type Attribute::DEFAULT_ATTRIBUTE before calling OpenSSL::PKey.read.
Also added a catch to exception OpenSSL::PKey::PKeyError to prevent a stack trace error if the passphrase was incorrect.

Defining an attribute without a default value generates a stacktrace
Signed-off-by: Omar J Irizarry <[email protected]>
@omar-irizarry omar-irizarry requested a review from a team as a code owner March 28, 2018 04:08
Signed-off-by: Omar J. Irizarry <[email protected]>
Copy link
Contributor

@jquick jquick left a comment

Choose a reason for hiding this comment

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

Thanks @omar-irizarry ! This is a really nice addition. Do you mind adding a unit test for this case?

if @passphrase.is_a? Inspec::Attribute::DEFAULT_ATTRIBUTE
return fail_resource 'Please provide default value for attribute'
end
begin
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also consider moving the logic out of the initialize method, so that we make sure it is a control error and its not happening during initialization.

Moved logic out of the initilize method.
Signed-off-by: Omar Irizarry <[email protected]>
Copy link
Contributor

@TrevorBramble TrevorBramble left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Have some small changes I'd like to see before merging. =^)

raise Inspec::Exceptions::ResourceFailed, 'passphrase Error'
end
key
end
Copy link
Contributor

Choose a reason for hiding this comment

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

You could restructure this to clarify the flow like this:

def read_pkey(filecontent, passphrase)
  default_attribute?(passphrase)

  OpenSSL::PKey.read(filecontent, passphrase)
rescue OpenSSL::PKey::PKeyError
  raise Inspec::Exceptions::ResourceFailed, 'passphrase error'
end

if passphrase.is_a? Inspec::Attribute::DEFAULT_ATTRIBUTE
raise Inspec::Exceptions::ResourceFailed, 'Please provide default value for attribute'
end
end
Copy link
Contributor

@TrevorBramble TrevorBramble Apr 9, 2018

Choose a reason for hiding this comment

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

Predicate methods (foo?) are expected to return true/false. I think it's right to pull out the test and exception raising into another method that can be used as a guard statement in read_pkey(). So maybe this method could be renamed ensure_not_default or raise_if_default or similar instead?

refactoring for better clarity.
Signed-off-by: Omar J Irizarry <[email protected]>
fixing trailing white spaces
Signed-off-by: Omar J Irizarry <[email protected]>
Copy link
Contributor

@TrevorBramble TrevorBramble left a comment

Choose a reason for hiding this comment

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

Thank you!

@jquick jquick changed the title Bug Fix #2865 Defining an attribute without a default value generates a stacktrace Require a key attribute for the key_rsa resource Apr 12, 2018
@jquick jquick added the Type: Enhancement Improves an existing feature label Apr 12, 2018
Copy link
Contributor

@jquick jquick left a comment

Choose a reason for hiding this comment

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

Thanks!

@jquick jquick merged commit a278ae9 into inspec:master Apr 12, 2018
@omar-irizarry omar-irizarry deleted the omar-irizarry/fix-issue-2865 branch April 26, 2018 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Improves an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants