Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
audit-logging: Send the command line with login #8130
Conversation
babbageclunk
changed the base branch from
develop
to
state-controller-refactor
Nov 27, 2017
| + ControllerTag: "controller-" + s.ControllerConfig.ControllerUUID(), | ||
| + ServerVersion: "2.3-rc2", | ||
| + } | ||
| + broken := make(chan struct{}) |
babbageclunk
Nov 27, 2017
Member
Will add one - took me a while to figure it out, may as well store it for next time!
| @@ -42,6 +44,7 @@ func (st *state) Login(tag names.Tag, password, nonce string, macaroons []macaro | ||
| Credentials: password, | ||
| Nonce: nonce, | ||
| Macaroons: macaroons, | ||
| + CLIArgs: utils.CommandString(os.Args...), |
wallyworld
Nov 27, 2017
Owner
We should check the back end version I suspect - we may need to go to Login v4?
We typically avoid sending data to the apiserver which it is not expecting.
babbageclunk
Nov 27, 2017
Member
Leaving the API at v3 was in the original spec - the thinking is:
Since the CLI args are entirely optional, and this isn't changing the behaviour of the Login method, I propose that we keep the Facade version at 3, and just add the CLIArgs to the params.LoginRequest structure. If a more modern client calls an older API server the extra argument is thrown away silently. An older client will not pass the value through, so the value will be empty. Both of these situations are fine.
wallyworld
Nov 27, 2017
Owner
And therein lies the issue - the modern client thinks the CLI args are being recorded and they are not, since the extra arg is silently discarded.
babbageclunk
Nov 27, 2017
Member
But is that a problem? It's not the client's concern whether the controller is doing audit logging, it's the controller's. If someone is running a controller that doesn't do audit logging, the more modern client shouldn't refuse to work.
babbageclunk
Nov 28, 2017
Member
For future reference, after more discussion I did try revving the Admin API. Unfortunately bumping the version specified for the Admin.Login call to 4 in the client meant that it couldn't connect to older controllers that only support v3 for Admin. The client can't use the usual version negotiation here because the LoginResult is what indicates supported versions for facades, so the solution would be to try v4 then fallback to v3. That's overkill in this case, so I'm leaving this as is.
howbazaar
Nov 28, 2017
Owner
I think that it is OK. From the server side it is optional, and if a client sends it and the server isn't expecting it, nothing untoward happens.
| @@ -529,6 +529,7 @@ type LoginRequest struct { | ||
| Credentials string `json:"credentials"` | ||
| Nonce string `json:"nonce"` | ||
| Macaroons []macaroon.Slice `json:"macaroons"` | ||
| + CLIArgs string `json:"cli-args"` |
| @@ -529,6 +529,7 @@ type LoginRequest struct { | ||
| Credentials string `json:"credentials"` | ||
| Nonce string `json:"nonce"` | ||
| Macaroons []macaroon.Slice `json:"macaroons"` | ||
| + CLIArgs string `json:"cli-args"` |
wallyworld
Nov 27, 2017
Owner
Why a string and not a slice?
I guess it's just to record what was done rather than being used anywhere?
babbageclunk
Nov 27, 2017
Member
Yes, that's right - it's just being captured so it can be logged, we don't need any structure.
babbageclunk
added some commits
Nov 22, 2017
babbageclunk
changed the base branch from
state-controller-refactor
to
develop
Nov 28, 2017
|
$$merge$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
jujubot
merged commit f440933
into
juju:develop
Nov 28, 2017
1 check passed
babbageclunk
deleted the
babbageclunk:login-cli-args
branch
Nov 28, 2017
babbageclunk
referenced this pull request
Dec 10, 2017
Merged
audit-logging: Send the command line with login #8196
added a commit
that referenced
this pull request
Dec 11, 2017
babbageclunk commentedNov 27, 2017
Description of change
Adds CLIArgs to LoginRequest, and populates it with os.Args when sending the login request from the client. (The controller doesn't do anything with it yet - eventually it'll write it into the audit log along with
facade method calls.)
This will let us tie API method calls to the command that was run to generate them.
QA steps
Run a series of commands against a controller with trace logging turned on. Check that the logged API requests include the cli-args key with the command that was run.