-
Notifications
You must be signed in to change notification settings - Fork 51
webconf service for the initial configuration of a device #143
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general comment it would be nice to have some sort of a finer grained split of the common bits between plain snapweb & webconf to avoid all that dup
cmd/snapweb/state.go
Outdated
@@ -0,0 +1,38 @@ | |||
/* | |||
* Copyright (C) 2014-2016 Canonical Ltd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2017
cmd/snapweb/state.go
Outdated
|
||
sysInfo, err := client.SysInfo() | ||
if err != nil { | ||
panic(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
panic or return false? I would go for the latter, the failure logic being the responsibility of the caller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return false is better, indeed
cmd/web-conf/main.go
Outdated
@@ -0,0 +1,182 @@ | |||
/* | |||
* Copyright (C) 2014-2017 Canonical Ltd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2017 only
cmd/web-conf/main.go
Outdated
|
||
const ( | ||
httpAddr string = ":4200" | ||
// httpsAddr string = ":4201" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove?
cmd/web-conf/main.go
Outdated
} | ||
|
||
// IsDeviceManaged determines if the device is in the 'managed' state | ||
func IsDeviceManaged() bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the duplication?
cmd/web-conf/main.go
Outdated
Transport: &http.Transport{Dial: unixDialer(socketPath)}, | ||
} | ||
|
||
log.Println(r.Method, r.URL.Path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove debug
run-spread-tests.sh
Outdated
echo "INFO: Creating new qemu test image(2) ..." | ||
(cd spread/image ; sudo ./create-image-embryonic.sh $channel) | ||
mkdir -p $SPREAD_QEMU_PATH | ||
mv -f spread/image/ubuntu-core-16-embryonic.img $SPREAD_QEMU_PATH/$image_name2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$image_name2 instead of ubuntu-core-16-embryonic.img
@@ -0,0 +1,132 @@ | |||
#!/bin/bash | |||
# | |||
# Copyright (C) 2016 Canonical Ltd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2017
www/src/js/models/create-user.js
Outdated
@@ -5,7 +5,7 @@ var Marionette = require('backbone.marionette'); | |||
var CONFIG = require('../config.js'); | |||
|
|||
module.exports = Backbone.Model.extend({ | |||
url: CONFIG.CREATE_USER, | |||
url: '/api/v2/create-user', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the change?
cmd/webconf/main.go
Outdated
|
||
func initURLHandlers(log *log.Logger, server net.Listener) { | ||
// API | ||
http.Handle("/api/", snappy.MakePassthroughHandler(dirs.SnapdSocket, "/api/")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes all of snapd available on the network.
No.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, totally : we only need /api/v2/create-user, as used in www/src/js/models/create-user.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please :-)
snappy/helpers.go
Outdated
<-sigchan | ||
waiter.Done() | ||
}() | ||
waiter.Wait() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems awfully contrived. You don't need the WaitGroup nor the goroutine at all; the whole function could just as well be
sigchan := make(chan os.Signal, 1)
signal.Notify(sigchan, syscall.SIGHUP)
<-sigchan
or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uh, well thanks: my go-fu will get better as a result
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry if I was harsh in the review; I meant it all kindly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh no, no worries really ;)
snappy/helpers.go
Outdated
} | ||
|
||
// MakePassthroughHandler maps a snapd API to a snapweb HTTP handler | ||
func MakePassthroughHandler(socketPath string, prefix string) http.HandlerFunc { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks like a terrible idea. Could you explain what you're trying to achieve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a general mechanism used in snapweb, and re-used as such in webconf, to map an HTTP call to the equivalent snapd API call. Instead of having a dedicated handler for each API in snapweb, and then linking the snapd client code to generate the unix call, Michael proposed to use this passthru, and minimize code duplication.
Note that In snapweb, this is only accessible if the access control token is sent along with the request.
sleep 2 | ||
|
||
echo "Verifying that snapweb now serves on port 4200" | ||
printf "GET / HTTP/1.1\nHost:localhost\nUser-Agent:nc\n\n" | nc localhost 4201 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4201?
printf "GET / HTTP/1.1\nHost:localhost\nUser-Agent:nc\n\n" | nc localhost 4201 | ||
|
||
# echo "The system should be managed now" | ||
# test `snap managed` = 'false' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
false?
test `snap managed` = 'false' | ||
|
||
echo "Verifying that webconf is running" | ||
test `ps ax | grep -- -linux-gnu/webconf | grep -v grep | wc -l` -eq 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd have to test, but I'd imagine ps h -C webconf
would work instead of ps | grep | grep
I think I addressed all comments. I have refactored the code a bit more to improve the test coverage. |
…em is already managed
Webconf provides a web UI for creating an initial administrative user on a device, similar to console-conf.
This initial implementation provides only the "create-user" part. Network configuration will come next.
A few words on the general architecture.
Webconf is implemented as a separate service, distinct from snapweb. Webconf runs only to turn an un-managed / embryonic device into a managed one. Once done, webconf is of no use on the device. At least, not until the device gets reset to its factory defaults.
Webconf is separated as a second safety measure over snapd's own guarantees against attempts at creating other admin users. In effect, snapd is protected by console access, whereas Snapweb only has the access token for it. Further, webconf is not protected by a token, and has to be available without preliminary console access, as its role is to open up console access for legit users. Hence the need to separate webconf and snapweb in distinct services.
Once webconf is done with the initial user creation, it stops and frees the HTTP socket for use by Snapweb. The handover is done by way of unix signals. Safety checks are placed at the start of both the webconf and snapweb services to ensure they won't run in the wrong context.
The UI for webconf, and its minimal daemon are implemented by re-using and re-packaging code from snapweb, since they both live in the same project space. Some further separation could be done at some point, in particular on the Web UI which is probably too complex for what it does. But i tried to stop when that was not helping anymore with the security of the implementation.
This branch also contains an initial set of spread tests verifying that webconf only runs on unmanaged devices. The tests also verify that snapweb doesn't start on an unmanaged device, and verify the transition between the 2 states.