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

add krb5 auth params for exit_koji_promote (auth to koji with krb5) #482

Merged
merged 2 commits into from Dec 8, 2016

Conversation

maxamillion
Copy link
Contributor

Signed-off-by: Adam Miller maxamillion@fedoraproject.org

Signed-off-by: Adam Miller <maxamillion@fedoraproject.org>
'use_auth', use_auth)
'use_auth', use_auth)

if self.spec.koji_use_kerberos.value:
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth enforcing that if koji_use_kerberos is set to True, principal and keytab values are also set? Alternatively, we could remove the koji_use_kerberos setting and only assume its behavior if koji_kerberos_principal and koji_kerberos_keytab are set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lcarva I'm good either way, the only reasdon I added koji_use_kerberos was there is a use_kerberos config flag already for the client and I was aiming for consistency. Which ever you and @twaugh (and Co.) prefer, I'm happy to go that direction.

Copy link
Member

Choose a reason for hiding this comment

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

If kerberos is in use for both, would the principal ever be different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@twaugh Yes, this koji auth is being internally used by the system for the koji content generator metadata import, the krb5 keytab is expected to be in the buildroot (either via an openshift secret or just drop it in the image). This is to configure exit_koji_promote while running "inner" in order to enable that for Fedora's use case because as of December 12th, ssl cert based auth will be completely disabled in Fedora's koji. (Do note that this getting merged and released by Dec 12th is not a blocker, I can patch our package in the internal Infrastructure dnf repos if needed so there's no real urgency here).

Copy link
Member

Choose a reason for hiding this comment

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

OK, so I prefer koji_use_kerberos and the koji_* keytab and principal parameters, and we just need to check that those parameters are set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@twaugh when you say "need to check" is there something not currently being done in this pull request that you would like to see done in the code or is that just a general comment?

Copy link
Member

Choose a reason for hiding this comment

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

From @lcarva's original comment, should we change this line to something like this?:

if (self.spec.koji_use_kerberos.value and
        self.spec.koji_kerberos_principal.value and
        self.spec.koji_kerberos_keytab.value):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ok. I follow. Thank you.

…fy args

Signed-off-by: Adam Miller <maxamillion@fedoraproject.org>
Copy link
Contributor

@lcarva lcarva left a comment

Choose a reason for hiding this comment

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

LGTM

@lcarva lcarva merged commit 82dacb3 into containerbuildsystem:master Dec 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants