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

Repos::ConjurCA doesn't raise a proper error when required secrets are empty #1315

Closed
orenbm opened this issue Feb 10, 2020 · 3 comments · Fixed by #1688
Closed

Repos::ConjurCA doesn't raise a proper error when required secrets are empty #1315

orenbm opened this issue Feb 10, 2020 · 3 comments · Fixed by #1688

Comments

@orenbm
Copy link
Member

orenbm commented Feb 10, 2020

Issue description

The class Repos::ConjurCA retrieves its required secrets by creating a Resource object and calling its last_secret method.

This is not good for supportability as in case the variable has no value we get a "NoMethodError: undefined method 'value' for nil:NilClass" error which doesn't tell the story.

In FetchRequiredSecrets we first verify that the variable exists and that it has a value before retrieving it. We should use this class in Repos::ConjurCA so we have this capability out of the box.

Steps to reproduce the issue

  1. Create an authn-k8s authenticator without loading a value into conjur/authn-k8s/<service-id>/ca/cert
  2. Run the authn-client to authenticate with k8s
  3. The Nil error will show in the logs

What's the expected result?

  • a RequiredResourceMissing or RequiredSecretMissing error is raised

What's the actual result?

  • a "NoMethodError: undefined method 'value' for nil:NilClass" error is raised
@alexkalish
Copy link
Contributor

@liavyona @orenbm: My knowledge about authn k8s troubleshooting is essentially non-existent. Could you please provide a more user focused description of the value that change provides? And please include an entry in the CHANGELOG. I'm assuming this is user facing, as an error class is changed and @InbalZilberman added it to the rolling release notes. Finally, I noticed that no tests were added/updated, which was a little surprising. Does this change need more coverage? Thanks!

@liavyona
Copy link
Contributor

liavyona commented Sep 3, 2020

Hi @alexkalish
Previously we used to fetch the needed values like that:

stored_cert = Resource[ca_info.cert_id].last_secret.value
stored_key = Resource[ca_info.key_id].last_secret.value

This is unsafe (might cause NoMethodError) due to:

  1. cert id / key_id / both didn't create at all => Resource[ca_info.cert_id] will be nil.
  2. cert id / key_id / both created but didn't initialize => Resource[ca_info.cert_id].last_secret will be nil.
    In my PR, we changed the relevant data fetching to use :Conjur::FetchRequiredSecrets.new which will raise a more clear error message (Errors::Conjur::RequiredSecretMissing).
    The scenarios described above are edge cases and even extreme ones.

@orenbm
Copy link
Member Author

orenbm commented Sep 3, 2020

@liavyona can you please add an entry under Fixed for this?
something like:

Verify that Kubernetes Authenticator variables exist and have value before retrieving them so that we raise a proper error if they aren't and enhance supportability

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

5 participants