Remove pbench-generate-token agent CLI functionality #3383
Conversation
There was a problem hiding this comment.
This came up in discussion this morning, and Peter was arguing against removing pbench-generate-token. While the normal way to get a Keycloak token is via the redirect tango, our functional tests are currently getting a token via Keycloak API. How practical this is in normal use, I don't know, given the bearer token lifetimes and refresh token handling: but do we want to reconsider this again??? 😕
But to do that we will have to expose the Keycloak APIs so the agent can call them. Can we do that with the integration lab Keycloak instance? Also is the integration lab Keycloak instance allow the CLI-based username-password authentication flow? |
You're the Keycloak expert here. The thread of the discussion was that the functional test integration shows it's possible. That doesn't necessarily mean it's feasible for general use, and if it depends on specific Keycloak configuration that might be a security issue, that's probably a problem. |
Yeah I mean it really depends on how the Keycloak instance in the integration lab is going to get configured. If we decide for security purposes that no one can login using the username/password through CLI (disallowing the password grant-type) then no we can't use the CLI to generate tokens. However, that brings another point, we have discussed that we (and also the integration lab instance) won't provide user registration which means all the user authentication has to happen through some identity providers. In this case, we anyway can not use the username/password on the CLI to generate the token (because our Keycloak broker is not an identity provider). Right? What we do for the functional test is just a workaround because 1. we can not use the dashboard in the functional test 2. Functional test user identity is not created using some identity provider, it was created by the broker. |
There was a problem hiding this comment.
But to do that we will have to expose the Keycloak APIs so the agent can call them.
At least in theory, we can write a CLI tool which does anything which the Dashboard does. (It might not be pretty or easy, but it should be just a "simple matter of programming".) In order to be able to push a result, the Agent already has to know where the Pbench Server is, so it can just ask the Pbench Server where the Keycloak service is (so that we don't have to "expose" the Keycloak any more than that).
Also is the integration lab Keycloak instance allow the CLI-based username-password authentication flow?
Presumably, the new pbench-token-generate would call the Pbench Server, then call the Keycloak broker, then handle the redirect to the OIDC identity provider, supply the username and password to it in a secure way, then ride the redirect back and obtain the access token and the refresh token, and finally hit the Pbench Server for an API key which it would return. (Or, perhaps it can return the access token, if the user can get the job done before it expires.)
I'm not thrilled by this model, because I think that API keys will be created rarely -- but used repeately -- and that we won't need a CLI tool to do it. (I.e, the user will create their key once and then copy it to wherever they need it, so it won't matter that there is no CLI to create it -- using the Dashboard will be sufficient, and maybe even better, because they won't need to install the Agent in the place where they do the one-time key creation.
it really depends on how the Keycloak instance in the integration lab is going to get configured.
It shouldn't...pbench-token-generate shouldn't be dependent upon any particular Pbench deployment configuration....
What we do for the functional test is just a workaround because
- we can not use the dashboard in the functional test
- Functional test user identity is not created using some identity provider, it was created by the broker.
I wouldn't call what we do in the functional test a "workaround" so much as an "alternate solution". We couldn't use the Dashboard because the functionality that we require isn't there. Once we have API keys, I expect that we'll switch most of the functional testing to use those, and we'll get away from authentication altogether (and just concentrate on authorization...). OIDC has freed us from the whole credentials biz, even in the Dashboard, thanks to all the redirection stuff, so it's just not a concern for us, now.
All that said, I got a couple of small requests for this PR, below.
| if [[ $# == 1 ]] | ||
| then | ||
| token=$1 | ||
| else | ||
| username=${1:-pbench_user} | ||
| password=${2:-pbench} | ||
| token=$(pbench-generate-token --username $username --password $password) | ||
| fi | ||
| # Get the Pbench authentication token | ||
| token_location=$1 | ||
| token=$(< ${token_location}) |
There was a problem hiding this comment.
Given that token_location is not used except for the line immediately following its definition, let's skip defining it.
Also, given that the token string is a required parameter and that script will behave in weird ways if it's omitted, you should add a check which exits with an error if it's not provided.
|
@webbnh I still do not understand how the user would be able to use the CLI to login when the identity provider is not the broker, it's either Redhat SSO or Google or something else. How to achieve these redirects without the presence of a browser and then securely insert user credentials on the identity provider login page? I guess theoretically, we can do it with curls or Python-requests but wouldn't it be messy? |
I doubt the path is gonna be that straight-forwards, with every functional test we create a new Keycloak container and a new database container. We create a brand new |
Now with that I disagree strongly. Maybe, if API Key and Keycloak auth token validation were 100% identical, we could get away with that. They're not, and we need to test both. So, we'll need to add API Key testing (whether a full duplicate test set or a few selected cases, including of course the That's not an issue, since I expect that our functional test environment will always be based off a local transient Keycloak ID provider that we completely control. |
Fundamentally, what the browser does is not magic. It just makes HTTP requests and renders the results. (If the result is Javascript or similar, then we start getting closer to magic, but for static pages it's just call-and-response.) If the response is a It's all doable. The question is, why would we do it? I maintain that an API key is, typically, something that the user will already have, so we don't need to invest a lot in letting them create one on the fly. Thus, I don't think we actually want to implement
You raise excellent points. We'll have to decide whether we want to create a new API key for each functional test run, or whether we find some way of persisting one, or provide some testing-specific approach.
My understanding of our current approach is that authentication/authorization is largely separate from the execution of the individual APIs, that either an access token or an API key is acceptable, and so by the time we reach the interesting part of the code, it doesn't matter which was originally specified. This is not to say that we don't need to test them both in specific cases -- obviously the authentication and authorization functions need to be appropriately tested -- but for the rest of the code we only need to use whichever one is more convenient (hence my original phrasing, "most of the functional testing" [emphasis added]). (And, yes, of course we'll need to test
😌 |
|
Moving it to draft until we decide the final strategy on this topic. |
I don't think the lack of a final strategy needs to gate this PR. At the most trivial, if we were to continue to provide a CLI command which fetches an access token or an API key, I don't think it will be called The paramount concern for this PR is that all the tests continue to pass. However, once you fix So, I don't know of any reason why this PR cannot move forward. (It's true that we will need to devise a method for testing each of access tokens and API keys, but I don't think we have to solve that in this PR.) |
Right, I just wanted some preliminary understanding of our thinking on how to move forward. At the very least the current functionality is anyway not useful since it relies upon the internal user notion which we have to remove anyway. And the new approach is drastically different and may need some experimentation before we implement it. |
- Since we moved away from the internal user management in Pbench-server, we can no longer use username and password to create new tokens. - The new functionality to generate long-lasting tokens will be available via new server API call and the dashboard. PBENCH-948
Yeah, I think we should just drop this |
Yes, I think its time has come and gone. |
PBENCH-948