WIP: Authentication #1946

Merged
merged 24 commits into from Jan 6, 2017

Projects

None yet

3 participants

@captncraig
Contributor

No description provided.

captncraig added some commits Sep 10, 2016
@captncraig captncraig Implementing https listener. Https and http may both be specified. Re…
…moving RelayListener.
548c61f
@captncraig captncraig Better default handling if https is specified and http is not 5f66a39
@captncraig captncraig initial sketch of web again 5f40e95
@captncraig captncraig more work on auth 4c34420
@captncraig captncraig auth closer to working a495dfe
@captncraig captncraig UI for creating tokens adad4b1
@captncraig captncraig slight change to token help 1ffb54c
@captncraig captncraig update gorilla. tweak e3529ce
@captncraig captncraig Merge branch 'master' into auth3
Conflicts:
	vendor/vendor.json
618bd0f
@captncraig captncraig vendor. activating redis
31c3d09
@captncraig captncraig work on stuff
12f8d76
@captncraig captncraig ui changes
5d6c67b
@captncraig captncraig default cookie secret.
aebff0a
@captncraig captncraig Merge branch 'master' into auth3
7901b63
@@ -31,7 +31,9 @@ import (
// outside of the package without a setter
type SystemConfProvider interface {
GetHTTPListen() string
- GetRelayListen() string
+ GetHTTPSListen() string
+ GetTLSCertFile() string
@kylebrandt
kylebrandt Nov 22, 2016 Member

PEM would be nice, or a interface that will allow for it in the future?

@captncraig
captncraig Nov 22, 2016 Contributor

This is to correspond to http.ListenAndServeTLS which expects two key file locations. They are pem encoded, yes, but maybe you mean a combined keyfile of some kind. There are other ways to host a tls server, but this is the easiest.

+ LdapAddr string
+ // allow insecure ldap connection?
+ AllowInsecure bool
+ // default permission level for anyone who can log in. Try "Reader".
@kylebrandt
kylebrandt Nov 22, 2016 Member

Where does "Reader" come from - what else should I try? :P

@captncraig
captncraig Nov 22, 2016 Contributor

It comes from the string parsing in roles.go. Any permission or role name should be valid, comma seperated.

cmd/bosun/main.go
@@ -284,6 +283,9 @@ func watch(root, pattern string, f func()) {
if wait.After(time.Now()) {
continue
}
+ if event.Name == "web/static.go" {
@kylebrandt
kylebrandt Nov 22, 2016 Member

What does this do? Add comment maybe?

@captncraig
captncraig Nov 22, 2016 Contributor

Stops the server from killing itself after esc runs. This was debugging that can probably be removed.

cmd/bosun/web/middlewares.go
+ "bosun.org/opentsdb"
+)
+
+// custom middlewares for bosun. Must match alice.Constructor signature (func(http.Handler) http.Handler)
@kylebrandt
kylebrandt Nov 22, 2016 Member

whitespace in comment

cmd/bosun/web/middlewares.go
+
+// custom middlewares for bosun. Must match alice.Constructor signature (func(http.Handler) http.Handler)
+
+var miniprofilerMiddleware = func(next http.Handler) http.Handler {
@kylebrandt
kylebrandt Nov 22, 2016 Member

miniProfiler? (casing)

cmd/bosun/web/middlewares.go
+type noopAuth struct{}
+
+func (n noopAuth) GetUser(r *http.Request) (*easyauth.User, error) {
+ //TODO: make sure ui sends header when possible, instead of json body
@kylebrandt
kylebrandt Nov 22, 2016 Member

outstanding TODO?

@captncraig
captncraig Nov 22, 2016 Contributor

not outstanding. todone.

cmd/bosun/web/middlewares.go
+
+func (n noopAuth) GetUser(r *http.Request) (*easyauth.User, error) {
+ //TODO: make sure ui sends header when possible, instead of json body
+ name := "anonymous"
@kylebrandt
kylebrandt Nov 22, 2016 Member

constant?

@captncraig
captncraig Dec 15, 2016 Contributor

nah, it gets changed.

cmd/bosun/web/middlewares.go
+
+func buildAuth(cfg conf.AuthConf) (easyauth.AuthManager, *token.TokenProvider, error) {
+ if cfg.CookieSecret == "" {
+ cfg.CookieSecret = "CookiesAreInsecure"
@kylebrandt
kylebrandt Nov 22, 2016 Member

constant again?

cmd/bosun/web/middlewares.go
+ authEnabled = true
+ // in proc token store so bosun can send data to itself with an ephemeral token
+ selfStore, _ := token.NewJsonStore("")
+ selfToken := token.NewToken(easyauth.RandomString(16), selfStore)
@captncraig
captncraig Nov 22, 2016 Contributor

16 bytes is an adequate hash secret. magic number because its not worth making a constant over.

@kylebrandt
kylebrandt Nov 22, 2016 Member

are you sure you want to justify a magic number on public record? ;-)

cmd/bosun/web/middlewares.go
+ tokensEnabled = true
+ data, ok := schedule.DataAccess.(redisStore.Connector)
+ if !ok {
+ return nil, nil, fmt.Errorf("web's data access does not implement correct redis connector interface")
@kylebrandt
kylebrandt Nov 22, 2016 Member

this an error that will bubble up to a user? If so - confusing.

@captncraig
captncraig Nov 22, 2016 Contributor

Hm. True. Was trying to make the redis connector interface somewhat orthogonal to the data interface. But this is an explicit dependency between them. May as well just add it to the public interface and cross that bridge if/when we implement another backend.

cmd/bosun/web/middlewares.go
+ return nil, nil, err
+ }
+ if cfg.AuthEnabled == false {
+ auth.AddProvider("nop", noopAuth{})
@kylebrandt
kylebrandt Nov 22, 2016 Member

log warn that there is no auth on start

@captncraig
captncraig Nov 22, 2016 Contributor

Good idea. Should probably link to docs on bosun.org. Once I add them.

+ Roles []bitDesc
+}
+
+func parseRole(s string) (easyauth.Role, error) {
@kylebrandt
kylebrandt Nov 22, 2016 Member

string parsing combined with auth scares me - is this our best option?

@captncraig
captncraig Nov 22, 2016 Contributor

This is just for converting roles in the config file to our numeric values. Should never be used at runtime. Maybe this should be somewhere in the config package or something?

cmd/bosun/web/embed.go
- go func() { io.Copy(os.Stdout, stdout) }()
- go func() { io.Copy(os.Stderr, stderr) }()
- if err := c.Wait(); err != nil {
+ log.Println("Starting")
@captncraig
captncraig Nov 22, 2016 Contributor

also debugging to remove

}
+
if schedule.SystemConf.SaveEnabled() {
@kylebrandt
kylebrandt Nov 22, 2016 Member

I like that we are more specific about the methods. But may need to note is as a break change if people are doing odd things.

@kylebrandt
Member

When I run Bosun without auth, I see my username and a log out drop down, even thought I am not logged in. This probably should not be visible in no auth mode:

image

@kylebrandt
Member

When running with a self-signed cert, the logs get spammed repeatedly with:

$ go run main.go -c bosun.toml -w -r -q -skiplast -n
2016/12/08 12:57:10 info: main.go:149: readonly relay at http://127.0.0.1:63606 to http://ny-tsdb02:4242
2016/12/08 12:57:10 info: web.go:224: tsdb host: 127.0.0.1:63606
2016/12/08 12:57:10 info: web.go:234: bosun web listening https on: :8080
2016/12/08 12:57:10 info: main.go:277: watching *.go in .
2016/12/08 12:57:10 info: main.go:277: watching *.html in web/static/templates
2016/12/08 12:57:10 info: main.go:277: watching *.ts in web/static/js
2016/12/08 12:57:25 error: queue.go:117: Post https://localhost:8080/api/put: x509: certificate signed by unknown authority

not sure why it is doing that with the -r flag anyways?

@kylebrandt
Member

Actually, that error may be a problem regardless:

2016/12/13 12:16:28 error: queue.go:117: Post https://localhost:8080/api/put: x509: certificate is valid for bosun, not localhost
@captncraig
Contributor

Ah yes. That does suck. I'm thinking we should just call the method directly if we are doing the in-proc mode. Perhaps a func in collect/metadata that can be swapped out to replace the http call altogether with a simple function call to the appropriate handler.

but datapoints really should be going out through tsdbrelay anyway. Holy shit this is a mess.

@kylebrandt
Member

Mixed content due to jquery: Mixed Content: The page at 'https://bosun:8080/login/' was loaded over HTTPS, but requested an insecure script 'http://code.jquery.com/jquery-3.1.1.min.js'. This request has been blocked; the content must be served over HTTPS.

@captncraig
Contributor

well that's just sloppy.

captncraig added some commits Dec 15, 2016
@captncraig captncraig some code review changes
6dab5ff
@captncraig captncraig compiling would be nice
646b23c
@captncraig captncraig metadata and collect allow insecure to localhost.
c007224
collect/queue.go
@@ -174,9 +176,15 @@ func SendDataPoints(dps []*opentsdb.DataPoint, tsdb string) (*http.Response, err
req.Header.Set("X-Access-Token", AuthToken)
}
Add("collect.post.total_bytes", Tags, int64(buf.Len()))
+ client := DefaultClient
+ if strings.Contains(tsdb, "localhost") {
@nicollet
nicollet Dec 19, 2016 Contributor

Just out of curiosity, can it also have "127.0.0.1"?

@kylebrandt
Member

Couple things regarding the bosun self metrics issue:

  • Is there some other communication channel we could use other than the HTTP route for the self metrics so we don't have to do this auth thing against localhost?
  • Unless you use influx or OpenTSDB, these metrics can't go anywhere. Might be good to wrap them under an option anyways
@captncraig captncraig Trying to do as much in proc as possible. Gets rid of several dirty h…
…acks.
fff4b44
metadata/metadata.go
func sendMetadata(ms []Metasend) {
+ if putFunction != nil {
@kylebrandt
kylebrandt Dec 22, 2016 Member

could dedent by inverting condition and then returning

+ if err != nil {
+ return nil, err
+ }
+ if resp.StatusCode == 401 {
@kylebrandt
kylebrandt Dec 22, 2016 Member

http.StatusUnauthorized constant

@@ -209,6 +208,15 @@ func Init(u *url.URL, debug bool) error {
return nil
}
+var putFunction func(k Metakey, v interface{}) error
+
+func InitF(debug bool, f func(k Metakey, v interface{}) error) error {
@kylebrandt
kylebrandt Dec 22, 2016 Member

Comment explaining InitF

captncraig added some commits Jan 4, 2017
@captncraig captncraig Merge commit 'cdcee2123b5071003a7c9c44b838018b0bd58929' into auth3
55b70c5
@captncraig captncraig fix 7f7ba47
@captncraig captncraig allowing username overrides for authorized users
0103abc
@captncraig captncraig ui tweaks
24e7508
@captncraig captncraig ui tweak
d334178
@captncraig captncraig ui tweak
eda1a36
@captncraig captncraig merged commit 67f50fd into master Jan 6, 2017

0 of 2 checks passed

bosun go generate needs to run.
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@captncraig captncraig deleted the auth3 branch Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment