overlord/devicestate: switch to ssh-keygen for device key generation #3130

vosst · Apr 3, 2017

Switch to ssh-keygen for device key generation.

Please note that placement of key files in /tmp is temporary and we are awaiting advice on where to put the files.

pedronis · Apr 3, 2017

pedronis reviewed Apr 3, 2017

View changes

some comments

we need to think where to best put these, though we know ssh-keygen will make sure they are root only accessible

an approach we use when we have things that invoke external commands is having:

sshGenerateKeyImpl this function
var sshGenerateKey = sshGenerateKeyImpl
MockSshGenarateKey

so most tests would Mock this function with something that doesn't fork/exec,
and a couple would test the real thing. It's worth only if the whole devicestate suite is much slower than on master

I did a couple of test runs and both master and this branch take mostly the same time, differences are in the range of milliseconds.

FWIW we also have testutil.MockCommand

we probably want to use osutil.OutputErr here:

out, err := cmd.CombinedOutput()
if err != nil {
return nil, osutil.OutputErr(out, err)
}

Thanks, fixed.

this is a left over from before noticing that Decode doesn't return an error actually

Thanks, fixed.

tyhicks · Apr 3, 2017

Hello - I was asked to take a look at this and was told that the change was for first-boot performance reasons on rpi2 and others.

I'm not a fan of this change since ssh-keygen is an external program and has a configuration file that we're not in control of. I feel like we should enable khwrng since most of these ARM chips have hardware random number generators and then take a better look into why Go's RSA keygen routine is taking so long.

I hear that @vosst is working on gathering some performance numbers between Go and OpenSSH keygen routines.

vosst · Apr 3, 2017

@tyhicks Thanks for your remarks. We are putting this PR up for review early as we treat it as a fallback right now. We already did the work of enabling hwrng and have it participate in the kernel's entropy gathering process. I'm deep diving into Go's rsa key generation algorithm and compare it to the one used by ssh-keygen.

ogra1 · Apr 3, 2017

@tyhicks even with the khwrng enabled the key generation in Go takes minutes while ssh-keygen takes seconds ... apparently there are some deep changes in Go needed to fix this as i understand. the ssh-keygen switch is only thought as an interim solution til Go itself can be fixed.

ogra1 · Apr 3, 2017

testable images with this change included are at http://people.canonical.com/~ogra/snappy/all-snaps/ssh-keygen-test/

setharnold · Apr 3, 2017

Is there any reason why 25519 keys aren't being generated instead? IIRC those are just 32 bytes of randomness with a few bits set specifically to ensure that the resulting key is on the curve. No primality testing, no probabilistic generation, etc.

Thanks

mwhudson · Apr 3, 2017

then take a better look into why Go's RSA keygen routine is taking so long

My understanding is that it's just less sophisticated. IIRC (and this maybe wrong), the performance of generating RSA keys is dominated by the performance of running the Miller-Rabin primality test which is dominated by the performance of Montgomery multiplication. openssl has hyper-optimized assembly montgomery multiplication routines, Go doesn't.

vosst · Apr 4, 2017

@mwhudson yup, I'm taking cpu profiles right now, and Go's BigInt operations that are repeatedly used in the primality tests seem to be the bottleneck here. I will follow up with more data.

See for example golang/go#15833.

Simple profiling is here: https://gist.github.com/vosst/cb3d16e1f1596baf6ccb989d6b1897a7
Results for x86_64 (Core i7): https://www.NoFile.io/f/2ExcAuaQpgO

ogra1 · Apr 4, 2017

http://paste.ubuntu.com/24312862/ has a set of captured data from 16 installs in a row (freshly flashed SD card each) with an image using this patch, the key generation with this takes between 10 and 20 seconds (compared to 5-15 minutes using the Go routine)

zyga · Apr 4, 2017

There's a failure in one of the tests. That test is pretty racy and I saw it fail many times recently:

FAIL: overlord_test.go:360: overlordSuite.TestEnsureLoopPrune

overlord_test.go:384:
    c.Assert(st.Change(chg1.ID()), Equals, chg1)
... obtained *state.Change = (*state.Change)(nil)
... expected *state.Change = &state.Change{state:(*state.State)(0xc820369dc0), id:"1", kind:"abort", summary:"...", status:0, clean:false, data:state.customData{}, taskIDs:[]string{"1"}, lanes:0, ready:(chan struct {})(0xc82020a6c0), spawnTime:time.Time{sec:63626902304, nsec:782578764, loc:(*time.Location)(0x110fa80)}, readyTime:time.Time{sec:63626902304, nsec:884419528, loc:(*time.Location)(0x110fa80)}}

zyga · Apr 4, 2017

There's some formatting errors as well:

Formatting wrong in following files:
/home/travis/gopath/src/github.com/snapcore/snapd/overlord/devicestate/handlers.go

I suspect you want to try gofmt -s (simplify)

vosst · Apr 4, 2017

Applied gofmt -s (did not know about -s).

chipaca · Apr 7, 2017

chipaca reviewed Apr 7, 2017

View changes

does rsa_generate_key take ownership of the strings? otherwise you're leaking them

Good catch, forgot to push the respective commit.

pedronis · Apr 7, 2017

pedronis reviewed Apr 7, 2017

View changes

why still using files on disk now?

Mainly because building up an rsa.PrivateKey is non-trivial and I'm using the parsing functions to get the job done. I haven't looked too deeply if I could leverage the openssl PEM encoding functions to place the encoded key into memory.

zyga · Apr 7, 2017

zyga reviewed Apr 7, 2017

View changes

Is this exhaustive? Do we need a default "unknown error" type case?

I modelled the enumeration as exhaustive as possible, aiming to capture all possible error conditions. For that, I left out the generic error code.

zyga · Apr 7, 2017

zyga reviewed Apr 7, 2017

View changes

How about calling this snapd_... so that various snapd C functions have a common prefix?

Done.

zyga · Apr 7, 2017

zyga reviewed Apr 7, 2017

View changes

I suspect this is undesired

Yes, fixed.

zyga · Apr 7, 2017

zyga reviewed Apr 7, 2017

View changes

Ditto for prefixing those with some snapd specific prefix.

Done.

pedronis · Apr 7, 2017

pedronis reviewed Apr 7, 2017

View changes

seems there is something like BIO_new_mem_buf we could use instead

Indeed, I adjusted the code to use it and avoid any sort of out-of-band file operations.

ogra1 · Apr 7, 2017

thomas asked me to do another round of tests after switching to direct openssl calls from shelling out to ssh-keygen ... http://paste.ubuntu.com/24334125/ has the results of 15 installs from the "Generating device key" step to "Mark system seeded". numbers do not seem much different from the ssh-keygen case, in average the key generation is around 10sec with 4sec being the fastest and about 30sec being the slowest.

pedronis · Apr 10, 2017

pedronis reviewed Apr 10, 2017

View changes

some comments

I would call this one generateRSAKey(keyLength int) ...
and at this point probably better to split this out also to a generate_rsa_key.go file

Done.

s/Failed to/cannot/, no final dot, same for all errors here

Done.

should be lowercase except for RSA etc, "...: could not"

Done.

cannot

Done.

this is more of a serialization/memory error now than I/O, no?

We cannot really tell. Might fail due to an issue in allocating memory by the underlying BIO memory writer.

we should check that ->memory is not NULL ?

Done.

pedronis · Apr 10, 2017

pedronis reviewed Apr 10, 2017

View changes

what's the purpose of this?

It's obsolete now, removed.

pedronis · Apr 10, 2017

pedronis reviewed Apr 10, 2017

View changes

thanks, a few more small comments

this applies also to the lib version?

can keyLength be just an int?

s/IO/MARSHAL/ s/persist/marshal/

otherwise it sounds this is about disk which is not the case anymore

Done.

this is something we should have done before, we can unlock around this:
st.Unlock()
keyPari, err := ...
st.Lock()

Done.

s/IO/MARSHAL/

Done.

tyhicks · Apr 10, 2017

tyhicks reviewed Apr 10, 2017

View changes

What directory are we in when creating this? Should an absolute path be specified instead of blindly going with the current working directory?

I will happily rely on security's guidance here.

I don't have a specific suggestion as I'm not sure what directory snapd typically uses for this sort of thing. Ideally, it should only be readable/writable by root.

@niemeyer any suggestions?

/run/snapd/ feels like the right place but I noticed that it isn't always created. For example, a test system of mine with no snaps installed does not have /run/snapd/ but it is created once a snap is installed.

I see it is referenced in dirs/dirs.go with the SnapRunNsDir variable. It is probably best that someone more active in snapd development advises you on if it is appropriate to use for your case.

this is moot, this function is used during the firstboot process of Ubuntu Core IoT images to generate the device key ... i dont think when using snapd on a desktop this function is used at all, you can be sure that /run/snapd exists and snapd is running at that point.

pedronis · Apr 11, 2017

pedronis dismissed their stale review Apr 11, 2017

PR changed again

pedronis · Apr 11, 2017

pedronis reviewed Apr 11, 2017

View changes

what ogra said is not fully true, we will use/might use this also on classic at some point, it's safer to create that dir here as well if it doesn't exist

Done.

zyga · Apr 11, 2017

zyga reviewed Apr 11, 2017

View changes

Do we properly depend on this in our packaging?

Both runtime and build time dependency list openssh-client (for Ubuntu at least).

zyga · Apr 11, 2017

@morphis FYI: potential new dependency for packaging

niemeyer · Apr 11, 2017

niemeyer requested changes Apr 11, 2017

View changes

A few trivial comments below.

Please get a review by @tyhicks on this one as well.

That's misleading. If we can't take < 1024, let's panic instead and force the test to read correctly.

Done.

This file would be better as crypto.go or similar, so it implies a better scope than this one function.

Done.

It would be better to have this as a dynamically named temporary file instead of a fixed one. Otherwise we can get hard to debug issues because of concurrently running logic.

Done.

This is strconv.Itoa.

Done.

Is the second parameter there an error?

If so, might be useful to have the hint of what was broken:

if err != nil {
        return nil, fmt.Errorf("cannot decode PEM block: %v", err)
}

The second parameter is the remainder of the input after decoding one block.

niemeyer · Apr 12, 2017

niemeyer approved these changes Apr 12, 2017

View changes

A couple of trivials, but LGTM anyway.

Thanks for all the work on this fix!

This may now be defer os.RemoveAll(tempDir) directly.

Done.

This reads slightly odd. Having them in their own var lines might be nicer in this particular case.

Done.

zyga · Apr 13, 2017

zyga approved these changes Apr 13, 2017

View changes

Looks good, I'll review remaining discussion and merge if all feedback is addressed.

Thank you for handling this detail :-)

Why do we unlock the state here? To run an external program?

@morphis ^^ FYI

vosst changed the title from overlord/devicestate: switch to keygen for device key generation to overlord/devicestate: switch to lib(open)ssl for device key generation Apr 10, 2017

vosst changed the title from overlord/devicestate: switch to lib(open)ssl for device key generation to overlord/devicestate: switch to ssh-keygen for device key generation Apr 10, 2017

pedronis dismissed their stale review Apr 11, 2017

+        +	deviceAPIBase           = deviceAPIBaseURL()
+        +	requestIDURL            = deviceAPIBase + "request-id"
+        +	serialRequestURL        = deviceAPIBase + "devices"
+        +	sshKeyFile              = "/tmp/snapd.key.tmp"

+        +	case C.RSA_KEY_GENERATION_IO_FAILURE:
+        +		return nil, errors.New("Failed to generate RSA key: Could not persist keys.")
+        +	case C.RSA_KEY_GENERATION_SUCCESS:
+        +		break

+        +	case C.SNAPD_RSA_KEY_GENERATION_SEED_FAILURE:
+        +		return nil, errors.New("Failed to generate RSA key: RNG not seeded.")
+        +	case C.SNAPD_RSA_KEY_GENERATION_ALLOCATION_FAILURE:
+        +		return nil, errors.New("Failed to generate RSA key: Could not allocate memory.")

+        +	defer C.free(unsafe.Pointer(privateKey.memory))
+        +	blk, _ := pem.Decode(C.GoBytes(unsafe.Pointer(privateKey.memory), C.int(privateKey.size)))
+        +	if blk == nil {
+        +		return nil, errors.New("Failed to decode PEM block")

+        +        // allocation functions and just free'ing it might not be correct.
+        +        BUF_MEM *buf_mem = NULL;
+        +        BIO_get_mem_ptr(bp_private, &buf_mem);
+        +        private_key->memory = malloc(buf_mem->length);

		)

		+func sshGenerateKey() (*rsa.PrivateKey, error) {

		)

		+func rsaGenerateKey() (*rsa.PrivateKey, error) {

snapcore/snapd

Join GitHub today

overlord/devicestate: switch to ssh-keygen for device key generation #3130

Conversation

vosst commented Apr 3, 2017 • edited Edited 1 time vosst Apr 3, 2017

vosst added some commits Apr 3, 2017

Some checks were not successful

pedronis reviewed Apr 3, 2017 View changes

pedronis Apr 3, 2017 • edited Edited 1 time pedronis Apr 3, 2017

tyhicks commented Apr 3, 2017

vosst commented Apr 3, 2017

ogra1 commented Apr 3, 2017

ogra1 commented Apr 3, 2017

setharnold commented Apr 3, 2017

vosst added some commits Apr 3, 2017

Some checks were not successful

mwhudson commented Apr 3, 2017

vosst commented Apr 4, 2017 • edited Edited 1 time vosst Apr 4, 2017

Some checks were not successful

ogra1 commented Apr 4, 2017

zyga commented Apr 4, 2017

zyga commented Apr 4, 2017

Some checks were not successful

vosst commented Apr 4, 2017

zyga and others added some commits Apr 5, 2017

All checks have passed

All checks have failed

chipaca reviewed Apr 7, 2017 View changes

All checks have failed

pedronis reviewed Apr 7, 2017 View changes

zyga reviewed Apr 7, 2017 View changes

zyga reviewed Apr 7, 2017 View changes

zyga reviewed Apr 7, 2017 View changes

zyga reviewed Apr 7, 2017 View changes

pedronis reviewed Apr 7, 2017 View changes

ogra1 commented Apr 7, 2017 • edited Edited 1 time ogra1 Apr 7, 2017

vosst added some commits Apr 7, 2017

All checks have failed

All checks have failed

All checks have failed

vosst changed the title from overlord/devicestate: switch to keygen for device key generation to overlord/devicestate: switch to lib(open)ssl for device key generation Apr 10, 2017

Some checks were not successful

pedronis reviewed Apr 10, 2017 View changes

pedronis Apr 10, 2017 • edited Edited 1 time pedronis Apr 10, 2017

pedronis reviewed Apr 10, 2017 View changes

vosst added some commits Apr 10, 2017

Some checks were not successful

pedronis reviewed Apr 10, 2017 View changes

vosst added some commits Apr 10, 2017

Some checks were not successful

All checks have failed

Some checks were not successful

All checks have failed

vosst changed the title from overlord/devicestate: switch to lib(open)ssl for device key generation to overlord/devicestate: switch to ssh-keygen for device key generation Apr 10, 2017

All checks have failed

tyhicks reviewed Apr 10, 2017 View changes

ogra1 Apr 10, 2017 • edited Edited 1 time ogra1 Apr 10, 2017

vosst added some commits Apr 11, 2017

All checks have failed

Some checks were not successful

pedronis dismissed their stale review Apr 11, 2017

pedronis reviewed Apr 11, 2017 View changes

pedronis Apr 11, 2017 • edited Edited 1 time pedronis Apr 11, 2017

zyga reviewed Apr 11, 2017 View changes

zyga commented Apr 11, 2017

vosst added some commits Apr 11, 2017

Some checks were not successful

niemeyer requested changes Apr 11, 2017 View changes

vosst added some commits Apr 11, 2017

All checks have passed

niemeyer approved these changes Apr 12, 2017 View changes

vosst added some commits Apr 12, 2017

All checks have passed

zyga approved these changes Apr 13, 2017 View changes

Hide details View details zyga merged commit b8e07b5 into snapcore:master Apr 13, 2017 6 checks passed

6 checks passed

vosst commented Apr 3, 2017 •

Edited 1 time

vosst
Apr 3, 2017

pedronis reviewed Apr 3, 2017

View changes

pedronis Apr 3, 2017 •

Edited 1 time

pedronis
Apr 3, 2017

vosst commented Apr 4, 2017 •

Edited 1 time

vosst
Apr 4, 2017

chipaca reviewed Apr 7, 2017

View changes

pedronis reviewed Apr 7, 2017

View changes

zyga reviewed Apr 7, 2017

View changes

zyga reviewed Apr 7, 2017

View changes

zyga reviewed Apr 7, 2017

View changes

zyga reviewed Apr 7, 2017

View changes

pedronis reviewed Apr 7, 2017

View changes

ogra1 commented Apr 7, 2017 •

Edited 1 time

ogra1
Apr 7, 2017

pedronis reviewed Apr 10, 2017

View changes

pedronis Apr 10, 2017 •

Edited 1 time

pedronis
Apr 10, 2017

pedronis reviewed Apr 10, 2017

View changes

pedronis reviewed Apr 10, 2017

View changes

tyhicks reviewed Apr 10, 2017

View changes

ogra1 Apr 10, 2017 •

Edited 1 time

ogra1
Apr 10, 2017

pedronis reviewed Apr 11, 2017

View changes

pedronis Apr 11, 2017 •

Edited 1 time

pedronis
Apr 11, 2017

zyga reviewed Apr 11, 2017

View changes

niemeyer requested changes Apr 11, 2017

View changes

niemeyer approved these changes Apr 12, 2017

View changes

zyga approved these changes Apr 13, 2017

View changes

zyga merged commit `b8e07b5` into snapcore:master Apr 13, 2017
6 checks passed