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

vdk-kerberos-auth: support kerberos auth for all CLI commands #774

Merged
merged 1 commit into from
Mar 24, 2022

Conversation

antoniivanov
Copy link
Collaborator

@antoniivanov antoniivanov commented Mar 20, 2022

vdk-control-cli commands also may require Kerberos authentication so
here we are adding support. Currently, we only authenticate during vdk
run (part of initialize_job). But with support for Kerberos for the
Control Service we'd need it for all operations. E.g vdk properties

Now authentication is always attempted as long as it's explicitly
enabled (KRB_AUTH is set to some valid value).
There's new flag "KRB_AUTH_FAIL_FAST" (false by default)
which would cause the plugin to fail if auth fails but otherwise only a warning would be logged.
It's necessary because only small subsets of features would use kerberos.
And vdk should work even if those feature do not.

Alternatives considered:

I was considering adding a new decorator/annotation to vdk-kerberos-auth
which "users" can use to denote that their method needs Kerberos authentication
(and would be authenticated on demand)

e.g

@kerberos 
def get_property(team, name): 

when get_property is called then Authenticator.authenticate is triggered. But for now I've discarded
a) it's more expensive
b) How do we deal with multiple auth mechanisms (as weh ave Oauth2 and kerberos) -
e.g when do we fail?, do we attempt both , etc.
c) caching of authentication
But it's nice idea and I am reserving it for the future.

Testing Done: extended the automated test of the plugin (see
test_kerberos).

Signed-off-by: Antoni Ivanov [email protected]

@antoniivanov antoniivanov force-pushed the person/aivanov/vdk-kerberos-auth branch 3 times, most recently from c6507b6 to aa54ffc Compare March 20, 2022 15:35
@antoniivanov antoniivanov marked this pull request as ready for review March 21, 2022 09:13
@antoniivanov antoniivanov force-pushed the person/aivanov/vdk-kerberos-auth branch from aa54ffc to 61f3ddc Compare March 21, 2022 11:30
@gabrielgeorgiev1
Copy link
Contributor

There's new flag "KRB_AUTH_FAIL_FAST" (false by default)
which would cause the plugin to fail if auth fails but otherwise only a warning would be logged.

I'm not clear I understand this in the desc. Do you mean that if the flag is true, an auth fail causes the whole execution of whatever VDK command is being ran to fail, and if it is false, it logs a warning but otherwise it proceeds normally? Perhaps rephrase it for clarity.

Copy link
Contributor

@gabrielgeorgiev1 gabrielgeorgiev1 left a comment

Choose a reason for hiding this comment

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

Looks good overall; I want clarity on some details before I can hit approve on this.

@antoniivanov antoniivanov force-pushed the person/aivanov/vdk-kerberos-auth branch 7 times, most recently from 6095a3f to 1eeb32b Compare March 24, 2022 08:00
vdk-control-cli commands also may require Kerberos authentication so
here we are adding support. Currently, we only authenticate during vdk
run (part of initialize_job). But with support for Kerberos for the
Control Service we'd need it for all operations. E.g `vdk properties`

Now authentication is always attempted as long as it's explicitly
enabled (KRB_AUTH is set to some valid value).
There's new flag "KRB_AUTH_FAIL_FAST" (false by default) which would
cause the plugin to fail if auth fails but otherwise only a warning
would be logged. It's necessary because only small subsets of features
would use kerberos. And vdk should work even if those feature do not.

Alternatives considered:

I was considering adding a new decorator/annotation to vdk-kerberos-auth
which "users" can use to denote that their method needs Kerberos
authentication (and would be authenticated on demand)

e.g
```
@kerberos
def get_property(team, name):
```

when get_property is called then Authenticator.authenticate is
triggered. But for now I've discarded
a) it's more expensive
b) How do we deal with multiple auth mechanisms (as weh ave Oauth2 and
kerberos) - e.g when do we fail?, do we attempt both , etc.
c) caching of authentication
But it's nice idea and I am reserving it for the future.

Testing Done: extended the automated test of the plugin (see
test_kerberos).

Signed-off-by: Antoni Ivanov <[email protected]>
@antoniivanov antoniivanov force-pushed the person/aivanov/vdk-kerberos-auth branch from 1eeb32b to 9c46fcc Compare March 24, 2022 08:39
@antoniivanov antoniivanov merged commit bceee9d into main Mar 24, 2022
@antoniivanov antoniivanov deleted the person/aivanov/vdk-kerberos-auth branch March 24, 2022 12:08
Copy link
Contributor

@ivakoleva ivakoleva left a comment

Choose a reason for hiding this comment

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

I am missing the exact problem description, seems to me the problem was __attempt_kerberos_authentication was only invoked during initialize_job, and now it's also invoked during vdk_initialize also. So it now applies to all vdk commands, not only vdk run.

Is it true that this feature is purposed to enable VDK-CLI-requesting data-job-specific endpoints (for example properties) using a keytab only?

I see two use-cases:

  1. a user who does not have valid oauth2 credentials, yet has gotten the data job keytab, and is now able to read data-job details, to operate/update/delete this data job;
  2. a data job now is able to manage itself via VDK CLI invocations (including reschedule, update it's own sources).

Use-case 1 is very rare - keytab is generated on vdk create, that runs after oauth2 authentication; the only way is for the keytab secret to be retrieved from someone having oauth2 credentials in the first place.
Use-case 2 does involve executing a vdk command, is that a recommendation and/or possibility? Should we document this?

@@ -13,10 +16,16 @@
KEYTAB_PRINCIPAL = "KEYTAB_PRINCIPAL"
KEYTAB_REALM = "KEYTAB_REALM"
KERBEROS_KDC_HOST = "KERBEROS_KDC_HOST"
KRB_AUTH_FAIL_FAST = "KRB_AUTH_FAIL_FAST"
Copy link
Contributor

Choose a reason for hiding this comment

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

So, the purpose of this change, is for vdk CLI to be able to also authenticate using a data job keytab

  • for data-job-specific endpoints (where name is present, so data-job-name.keytab is auto-located within dir configured)
  • in addition to personal oauth2 credentials, correct?
  • with precedence to oauth2 credentials

And, this KRB_AUTH_FAIL_FAST toggle set to false by default, would only notify user of potential kerberos auth issues then fallback to oauth2?
Is it a correct statement that kerberos auth has precedence over oauth2 when using VDK CLI? Should that be configurable, or described?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it a correct statement that kerberos auth has precedence over oauth2 when using VDK CLI? Should that be configurable, or described?

That will be a separate change in vdk-control-cli which would decide how would both work.

This change just make it sure to kinit during vdk_initialize so that other commands (not just vdk run) can re-use the generated Kerberos ticket (TGT) if they need to.

@antoniivanov
Copy link
Collaborator Author

A use-case is to enable Control Service deployments that use only Kerberos as authentication (without Oauth2).

If the Control Service supports Kerberos but the vdk cli does not, it is not complete. It's important to keep the client (vdk-control-cli) and server (Control Service REST API) in sync.

@ivakoleva
Copy link
Contributor

A use-case is to enable Control Service deployments that use only Kerberos as authentication (without Oauth2).

If the Control Service supports Kerberos but the vdk cli does not, it is not complete. It's important to keep the client (vdk-control-cli) and server (Control Service REST API) in sync.
@tozka

In general I agree, and in favour of consistency. Yet, Kerberos auth is mainly targeted for an actor being a data job (in case of a person, that is a very rare corner case). Does that mean that VDK CLI command is now purposed for evaluation from within a data job context? For example, a data job would be evaluating VDK CLI commands?
And if yes, maybe we need to document this design/use-case/guideline?

Also, isn't another approach more suitable for facilitating such an use case (data job requesting the REST API), rather than the CLI? Perhaps the vdk-plugin-control-cli instead?

What I'm missing, is the use-cases first, then the implementation

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