Skip to content
This repository has been archived by the owner on Oct 4, 2023. It is now read-only.

webconf service for the initial configuration of a device #143

Open
wants to merge 55 commits into
base: master
Choose a base branch
from
Open

webconf service for the initial configuration of a device #143

wants to merge 55 commits into from

Conversation

dbarth
Copy link

@dbarth dbarth commented Jan 30, 2017

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.

Copy link
Contributor

@AlexandreAbreu AlexandreAbreu left a 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

@@ -0,0 +1,38 @@
/*
* Copyright (C) 2014-2016 Canonical Ltd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2017


sysInfo, err := client.SysInfo()
if err != nil {
panic(err)
Copy link
Contributor

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

Copy link
Author

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

@@ -0,0 +1,182 @@
/*
* Copyright (C) 2014-2017 Canonical Ltd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2017 only


const (
httpAddr string = ":4200"
// httpsAddr string = ":4201"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove?

}

// IsDeviceManaged determines if the device is in the 'managed' state
func IsDeviceManaged() bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the duplication?

Transport: &http.Transport{Dial: unixDialer(socketPath)},
}

log.Println(r.Method, r.URL.Path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove debug

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
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2017

@@ -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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the change?

@dbarth dbarth requested a review from chipaca February 20, 2017 15:42

func initURLHandlers(log *log.Logger, server net.Listener) {
// API
http.Handle("/api/", snappy.MakePassthroughHandler(dirs.SnapdSocket, "/api/"))
Copy link
Contributor

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.

Copy link
Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please :-)

<-sigchan
waiter.Done()
}()
waiter.Wait()
Copy link
Contributor

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?

Copy link
Author

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

Copy link
Contributor

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

Copy link
Author

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 ;)

}

// MakePassthroughHandler maps a snapd API to a snapweb HTTP handler
func MakePassthroughHandler(socketPath string, prefix string) http.HandlerFunc {
Copy link
Contributor

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?

Copy link
Author

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
Copy link
Contributor

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'
Copy link
Contributor

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
Copy link
Contributor

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

@dbarth
Copy link
Author

dbarth commented Mar 2, 2017

I think I addressed all comments. I have refactored the code a bit more to improve the test coverage.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants