cmd/juju/user: correct padding for user registration string #7028

Merged
merged 5 commits into from Feb 27, 2017

Conversation

Projects
None yet
3 participants
Contributor

4a6f656c commented Feb 23, 2017

The current code incorrectly pads the ASN.1 encoded user registration
data with remainder bytes, rather than padding it to a multiple of
three bytes. This results in equal signs being appended for base64
padding.

Also add test coverage that verifies the registration string only
consists of alphanumeric characters.

This looks great, thanks! Could you change the PR to be against develop? The merge bot won't merge things into staging directly. (I'm not sure why that's the default when we create PRs.)

@howbazaar howbazaar changed the base branch from staging to develop Feb 24, 2017

Member

babbageclunk commented Feb 24, 2017

Actually, I got @howbazaar to do it.

$$merge$$

Member

babbageclunk commented Feb 24, 2017

Let's try now.

$$merge$$

Contributor

jujubot commented Feb 24, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

Contributor

jujubot commented Feb 24, 2017

Build failed: Generating tarball failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10363

Member

babbageclunk commented Feb 24, 2017

gofmt is unhappy with this - line 118 of add_test.go should be:

s.mockAPI.secretKey = []byte(strings.Repeat("X", 32+i))

(No spaces in 32+i.)

I'd fix it but I don't have the privs required to edit the PR.

cmd/juju/user: correct padding for user registration string
The current code incorrectly pads the ASN.1 encoded user registration
data with remainder bytes, rather than padding it to a multiple of
three bytes. This results in equal signs being appended for base64
padding.

Also add test coverage that verifies the registration string only
consists of alphanumeric characters.
Contributor

4a6f656c commented Feb 24, 2017

@babbageclunk Ugh, fixed - thanks!

Member

babbageclunk commented Feb 27, 2017

Sorry, missed that this was updated.

$$merge$$

Contributor

jujubot commented Feb 27, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit ca1ac3f into juju:develop Feb 27, 2017

@4a6f656c 4a6f656c deleted the 4a6f656c:juju-register-padding branch Mar 1, 2017

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