[KIALI-670]Token #218

aljesusg · 2018-05-28T09:53:57Z

Changed to JWT Working in expired at

xeviknal

I think it is pretty simple but powerful implementation of the token management. LGTM!

jotak · 2018-05-28T10:16:31Z

config/security/token.go

+func (ti *TokenInfo) ToBase64() string {
+	bytes, err := json.Marshal(ti)
+	if err != nil {
+		log.Errorf("Failed to marshal TokenInfo.")


Can you return an error here?

jotak · 2018-05-28T10:48:56Z

handlers/root.go

+		RespondWithJSONIndent(w, http.StatusInternalServerError, nil)
+		return
+	}
+	token := security.NewAuthToken(u+p, time.Now().Local().Add(time.Hour*time.Duration(config.Get().Token.ExpirationHours)), config.Get().Token.Secret)


Isn't the classical username+password check missing here, before generating the token?

I decided to generate token from username + password. The check is u, p, ok := r.BasicAuth()

But r.BasicAuth doesn't actually check anything, does it? I mean, we still need to make sure it matches somehow the username and password we have in our config file, conf.Server.Credentials.Username and conf.Server.Credentials.Password

jotak · 2018-05-28T11:07:49Z

config/config.go

@@ -144,6 +153,10 @@ func NewConfig() (c *Config) {
 	c.Products.Istio.IstioSidecarAnnotation = strings.TrimSpace(getDefaultString(EnvIstioSidecarAnnotation, "sidecar.istio.io/status"))
 	c.Products.Istio.UrlServiceVersion = strings.TrimSpace(getDefaultString(EnvIstioUrlServiceVersion, "http://istio-pilot:9093/version"))

+	// Token Configuration
+	c.Token.Secret = strings.TrimSpace(getDefaultString(EnvTokenSecret, "kiali"))
+	c.Token.ExpirationHours = getDefaultInt(EnvTokenExpirationHours, 2)


Higher default for convenience, like 10 hours?

jotak · 2018-05-28T11:08:13Z

config/security/token.go

+	}
+}
+
+func (at *AuthToken) GetTokenInfo(secret string) (*TokenInfo, error) {


Trying to find a flaw, but it seems good :)
However I wonder if we could use an existing JWT-like implementation that would be more widely tested & trusted.

keycloak not support golang I didn't find any JWT implementation in golang only some methods of thirds....

I'd also prefer an existing JWT implementation.
This page list several ones for golang: https://jwt.io/

This is while we add the authentication against openshift that will be this Sprint, I don't know if it's a good idea spent time in work in a full JWT implementation @israel-hdez

Mmm there are only libraries of thirds...I am gonna check them

aljesusg · 2018-05-28T11:09:52Z

@lucasponce @jotak @xeviknal review the token authentication please

jotak · 2018-05-28T11:10:32Z

@aljesusg overall looks good. I'd like others to review as well as security is critical.

aljesusg · 2018-05-28T11:40:04Z

I did changes @xeviknal @lucasponce @jotak I don't know if it's necessary to spent time implementing jwt token when this is going to change soon. But If you think different, I'll do it

jotak · 2018-05-28T11:41:22Z

handlers/root.go

+		RespondWithJSONIndent(w, http.StatusInternalServerError, nil)
+		return
+	}
+	token, err := security.NewAuthToken(u+p, time.Now().Local().Add(time.Hour*time.Duration(config.Get().Token.ExpirationHours)), config.Get().Token.Secret)


Not sure why my previous comment went outdated, I was saying:

r.BasicAuth doesn't actually check anything. I mean, we still need to make sure it matches somehow the username and password we have in our config file, conf.Server.Credentials.Username and conf.Server.Credentials.Password . Else I bet you can login with any credential :)

I don't know If I am udnerstanding you.
In server.go L-87 is checking that the username and password are the same

else if h.credentials.Username != "" || h.credentials.Password != "" { u, p, ok := r.BasicAuth() if !ok || h.credentials.Username != u || h.credentials.Password != p { statusCode = http.StatusUnauthorized }

h is called in the handler where proxyHandler have the config file credentials

proxyHandler := serverAuthProxyHandler{ credentials: security.Credentials{ Username: conf.Server.Credentials.Username, Password: conf.Server.Credentials.Password, }, trueHandler: router, } http.HandleFunc("/", proxyHandler.handler)

So if a try to access with another credentials fail

Ok my bad, I didn't see the whole picture. If the authorization handler is still called in addition to the new one then I suppose it's fine! 👍

aljesusg · 2018-05-28T18:21:39Z

Changed to JWT Working in expired at

aljesusg · 2018-05-28T18:24:09Z

Maybe we should add in another PR the Expired time because there is a JIRA for that https://issues.jboss.org/browse/KIALI-533 .

What do you think @israel-hdez @jotak @xeviknal @lucasponce ?

josejulio · 2018-05-28T21:47:42Z

server/server.go

@@ -78,7 +78,13 @@ func (h *serverAuthProxyHandler) handler(w http.ResponseWriter, r *http.Request)
 	statusCode := http.StatusOK

 	// before we handle any requests, make sure the user is authenticated
-	if h.credentials.Username != "" || h.credentials.Password != "" {
+	if r.Header.Get("Kiali-Token") != "" {


JWT is usually used (not a requirement, but i have seen that most services do this way) in the header 'Authorization' and usually has the format "Bearer the_token"
See jwt.io [1] and jira docs, wikipedia, etc.

I think we should comply unless there is a reason we can't do that.

[1]

Whenever the user wants to access a protected route or resource, the user agent should send the JWT, typically in the Authorization header using the Bearer schema. The content of the header should look like the following:

Authorization: Bearer <token>

josejulio · 2018-05-28T22:08:12Z

config/token.go

+		fmt.Println("Here")
+		return err
+	}
+	if token.Valid {


I think you are missing an step. Validate the "alg" claim. (taken from here)

// Don't forget to validate the alg is what you expect: if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok { return nil, fmt.Errorf("Unexpected signing method: %v", token.Header["alg"]) }

If not validated, one could create a JWT with the "alg = none" and bypass your protection.
alg none means that the jwt is unsecured, but is still a valid JWT, and the RFC requires [1] that the libs implement that one.

See this post [2] for more info.

The none algorithm is a curious addition to JWT. It is intended to be used for situations where the integrity of the token has already been verified. Interestingly enough, it is one of only two algorithms that are mandatory to implement (the other being HS256).

[1] https://tools.ietf.org/html/rfc7519#section-8
[2]https://auth0.com/blog/critical-vulnerabilities-in-json-web-token-libraries/

We could also configure validMethods

Well, it seems that none is disabled by default https://github.com/dgrijalva/jwt-go/blob/master/none.go#L28
But anyway, we should only allow tokens signed with SigningMethodHS256

josejulio · 2018-05-28T22:24:29Z

config/token.go

+
+type TokenGenerated struct {
+	Token     string `json:"token"`
+	ExpiredAt string `json:"expired_at"`


The Expiration Time is a standard JWT claim [1], I don't think we should duplicate this value, but when parsing we could "unwrap" into a more friendly/verbose format if we want to use the contained data.

Besides, "Token" is signed and encoded, not encrypted, if consumers want to know their contents (claims) they could decode (using base64) the second segment or use a JWT lib, and for debugging purposes or quickly having a look at their values, you can use this site: https://jwt.io/ and paste the token there.

[1] https://tools.ietf.org/html/rfc7519#section-4.1.4

We wanna set a expiration time only @josejulio in the claim against the configuration file

Not a blocker, but this field is duplicated, set once in the token (encoded) and once in the answer of GenerateToken (verbose).

If you see in the handlerRoot you can see that Token generated is the object returned to the user, in tokenclaim is the info to generate the token and the second one the output to the user where I inform him about when the token will be expired

Yes, i see that, i only mean to say that the expiresAt is also self-contained in the token (the unix timestamp) and the consumer can use that too, we don't have to pull out every data from the claims.
Again, not a blocker, but would like to make that clear in case we want to add more data to the claims.

I did it by https://issues.jboss.org/browse/KIALI-533 for the sessions time out, but you are right about get the expired against the token

josejulio · 2018-05-28T23:08:10Z

Maybe we should add in another PR the Expired time because there is a JIRA for that https://issues.jboss.org/browse/KIALI-533 .

Is just matter of adding the proper claim to the JWT (check my other comments) and the validation is done by the JWT lib

aljesusg · 2018-05-29T06:37:14Z

Thanks @josejulio for comments, is still in WIP ^^

aljesusg · 2018-05-29T09:58:38Z

This is ready with ExpiredAt working too.

Can you review this @jotak @lucasponce @xeviknal ?

lucasponce · 2018-05-29T12:17:49Z

config/token.go

+	"fmt"
+	"github.com/dgrijalva/jwt-go"
+	"github.com/kiali/kiali/log"
+	"time"


minor, but @jmazzitelli will be happy if we order the imports in
standard imports
LINE
third party imports
LINE
kiali imports

Changed, we should put this in the future style code guide ^^ Thanks @lucasponce

lucasponce · 2018-05-29T12:22:12Z

config/token.go

+		} else if ve.Errors&(jwt.ValidationErrorExpired|jwt.ValidationErrorNotValidYet) != 0 {
+			// Token is either expired or not active yet
+			log.Debugf("Timing is everything")
+			return errors.New("Timing is everything")


"Timing is everything" sounds like extracted from Thanos quotes. :-)

Not a blocker, it's funny.

Hahahaa Sorry ^^ I should put anothet kind of message xD

Maybe "Token expired ... Timing is everything"

lucasponce

LGTM but I would let @josejulio to take a look before to merge.

lucasponce · 2018-05-29T12:24:00Z

handlers/root.go

+		RespondWithJSONIndent(w, http.StatusInternalServerError, u)
+		return
+	}
+	fmt.Printf("Token " + u)


This print shouldn't be a log.info if it is intentional print not debug line ?

lucasponce · 2018-05-29T12:24:15Z

handlers/root.go

+	fmt.Printf("Token " + u)
+	token, error := config.GenerateToken(u)
+	if error != nil {
+		fmt.Printf("Error fine %+v", error)


lucasponce · 2018-05-29T12:25:27Z

routing/routes.go

@@ -30,6 +30,12 @@ func NewRoutes() (r *Routes) {
 			"/api",
 			handlers.Root,
 		},
+		{ // another way to get to root, both show status


/api/token gets a token or status ?

Changed comment

lucasponce · 2018-05-29T12:25:41Z

server/server.go

@@ -8,6 +8,7 @@ import (
 	"github.com/kiali/kiali/config/security"
 	"github.com/kiali/kiali/log"
 	"github.com/kiali/kiali/routing"
+	"strings"


wrong import order

Changed ^^ Jetbrains autoimport ^^

lucasponce · 2018-05-29T12:27:24Z

server/server.go

+	if strings.Contains(r.Header.Get("Authorization"), "Bearer") {
+		err := config.ValidateToken(strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer "))
+		if err != nil {
+			log.Debugf("Error token %+v", err)


I would move this to Info/Warn.
I guess I would like to see in the log bad authentication entries.

Changed to Warning

jotak · 2018-05-29T14:08:07Z

config/config.go

+	if retValString == "" {
+		retVal = defaultValue
+	} else {
+		if num, err := strconv.ParseInt(retValString, 2, 64); err != nil {


In base 2? Is it intentional?

Nope ^^ mindblind

israel-hdez · 2018-05-29T13:59:55Z

config/config.go

@@ -92,6 +95,11 @@ type Products struct {
 	Jaeger               JaegerConfig  `yaml:"jaeger,omitempty"`
 }

+type Token struct {
+	Secret       []byte `yaml:"secret,omitempty"`


Hmm... Probably we should add a warning in Readme regarding filesystem permissions now that the config file can have the secret.

Added the Documentation about the Secret

aljesusg · 2018-05-29T16:32:48Z

@israel-hdez @josejulio can we merge this now?

aljesusg · 2018-05-29T16:41:21Z

Thanks @josejulio

jotak · 2018-05-29T16:46:35Z

Thanks for the good work @aljesusg !

aljesusg requested review from xeviknal, lucasponce and jotak May 28, 2018 09:53

aljesusg force-pushed the token_auth branch 2 times, most recently from 4b4b24e to a1f0ced Compare May 28, 2018 09:55

xeviknal previously approved these changes May 28, 2018

View reviewed changes

aljesusg changed the title ~~[WIP]Token~~ [KIALI-670]Token May 28, 2018

aljesusg dismissed xeviknal’s stale review via 250e4c7 May 28, 2018 10:56

aljesusg force-pushed the token_auth branch from a1f0ced to 250e4c7 Compare May 28, 2018 10:56

jotak reviewed May 28, 2018

View reviewed changes

aljesusg force-pushed the token_auth branch from 2b96303 to b17dd2a Compare May 28, 2018 11:21

jotak reviewed May 28, 2018

View reviewed changes

aljesusg added the do not merge A PR is not ready to merge label May 28, 2018

aljesusg mentioned this pull request May 28, 2018

[KIALI-670] Add jwt golang library #223

Merged

aljesusg force-pushed the token_auth branch from b17dd2a to 42df4ca Compare May 28, 2018 18:20

josejulio reviewed May 28, 2018

View reviewed changes

aljesusg force-pushed the token_auth branch from 42df4ca to 91e1bac Compare May 29, 2018 09:54

aljesusg removed the do not merge A PR is not ready to merge label May 29, 2018

aljesusg mentioned this pull request May 29, 2018

[KIALI-534] Login kiali/kiali-ui#314

Merged

lucasponce reviewed May 29, 2018

View reviewed changes

aljesusg force-pushed the token_auth branch from 91e1bac to 152e1fa Compare May 29, 2018 12:20

lucasponce reviewed May 29, 2018

View reviewed changes

aljesusg force-pushed the token_auth branch 2 times, most recently from 6f53174 to 824ec38 Compare May 29, 2018 12:26

lucasponce previously approved these changes May 29, 2018

View reviewed changes

aljesusg dismissed lucasponce’s stale review via 1eb79db May 29, 2018 12:32

aljesusg force-pushed the token_auth branch from 824ec38 to 1eb79db Compare May 29, 2018 12:32

jotak reviewed May 29, 2018

View reviewed changes

[KIALI-670] Add JWT Token Authentication

44711eb

aljesusg force-pushed the token_auth branch from 1eb79db to 44711eb Compare May 29, 2018 14:18

josejulio previously approved these changes May 29, 2018

View reviewed changes

israel-hdez reviewed May 29, 2018

View reviewed changes

Added warning documentation about the Secret

ec21214

aljesusg dismissed josejulio’s stale review via ec21214 May 29, 2018 16:25

josejulio approved these changes May 29, 2018

View reviewed changes

josejulio merged commit dec255b into kiali:master May 29, 2018

[KIALI-670]Token #218

[KIALI-670]Token #218

Conversation

aljesusg commented May 28, 2018 • edited

xeviknal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aljesusg commented May 28, 2018 • edited

jotak commented May 28, 2018

aljesusg commented May 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aljesusg commented May 28, 2018

aljesusg commented May 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josejulio May 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josejulio commented May 28, 2018

aljesusg commented May 29, 2018

aljesusg commented May 29, 2018

lucasponce May 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasponce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aljesusg commented May 29, 2018

aljesusg commented May 29, 2018

jotak commented May 29, 2018

aljesusg commented May 28, 2018 •

edited

aljesusg commented May 28, 2018 •

edited

josejulio May 28, 2018 •

edited

lucasponce May 29, 2018 •

edited