Implement auth #186

setrofim · 2023-08-21T14:22:15Z

Implement API authentication and authorization.

Define IAuthorizer interface that can be used to obtain a gin
middleware handler that performs authorisation.
Add a mechanism to obtain an IAuthorizer for a particular role base on
Veraison configuration.
Implement "passthrough" authorizer that duplicates existing behavior
(no auth).
Implement "basic" authorizer that does not rely on an external
authorization server.
Implement "keycloak" authorizer that uses Keycloak service for
authentication.
Update provisioning service to authorize based on "provisioner" role.
Update management service to authorize based on "manager" role.
Add the previously missing README for the management service.

note: Once the corresponding client-side support for auth has been added, there will be additional changes to update the colci/ pocli inside the docker deployment.

deployments/docker/veraison

auth/README.md

auth/iauthorizer.go

auth/basic.go

deployments/docker/src/keycloak.conf

yogeshbdeshpande · 2023-08-30T13:47:19Z

deployments/docker/src/keycloak.docker

+WORKDIR /opt/keycloak
+# note: for development set up early; use proper certification in production.
+RUN keytool -genkeypair -storepass password -storetype PKCS12 -keyalg RSA -keysize 2048 \
+	-dname "CN=server" -alias server -ext "SAN:c=DNS:localhost,IP:127.0.0.1" \


should the server IP on line 6 be hard coded or read from yaml file?

nah, this isn't really used. This sets up keys used for the HTTPS cert, but we're enabling and using HTTP (specifically to avoid having to deal with certs in the test deployment). There isn't really a way to disable HTTPS (that I've seen), so the keys need to generated even if they're not used.

yogeshbdeshpande · 2023-08-30T13:48:52Z

deployments/docker/src/load-config.mk

@@ -11,6 +11,7 @@ VTS_PORT ?= 50051
 PROVISIONING_PORT ?= 8888
 VERIFICATION_PORT ?= 8080
 MANAGEMENT_PORT ?= 8088
+KEYCLOAK_PORT ?= 11111


Just a thought, this could be 8880 ?

I specifically chose something different, since this isn't really part of Veraison -- this is an external service Veraison depends on (besides, its already difficult enough to remember the correct port to use for DEBUG_PORT, as they're all just 8's and 0's in various permutations).

provisioning/cmd/provisioning-service/README.md

yogeshbdeshpande · 2023-08-30T13:58:44Z

provisioning/cmd/provisioning-service/main.go

@@ -73,8 +74,19 @@ func main() {
 	provisioner := provisioner.New(vtsClient)

 	log.Infow("initializing provisioning API service", "address", cfg.ListenAddr)
+	authorizer, err := auth.NewAuthorizer(subs["auth"], log.Named("auth"))


Presumably if the Line 41 auth subs is missing the NewAuthroizer will default to passthrough ?

Yup: https://github.com/veraison/services/blob/setrofim-auth/auth/authorizer.go#L21

yogeshbdeshpande

Left some minor comments but in general LGTM!

setrofim · 2023-08-31T08:31:29Z

Comments addressed.

auth/README.md

Signed-off-by: Sergei Trofimov <[email protected]>

thomas-fossati

LGTM, thanks!

thomas-fossati · 2023-09-05T07:45:48Z

auth/basic.go

+			c.Writer.Header().Set("WWW-Authenticate", "Basic realm=veraison")
+			c.AbortWithStatus(http.StatusUnauthorized)


(just a suggestion)

To make this a bit more user friendly (i.e., to let the user distinguish among the different 401 root causes) we could return a "problem details" payload like we do elsewhere.

(this would apply to all other 401 returns in this scope.)

Good point. Will add that.

Incidentally, we really need to think about how we're organising our code. There is enough commonality between the different REST API (verification, provisioning, etc) implementations, such as the ReportProblem, that it would make sense to have a common place for the code, rather than duplicationg it for each API.

Good point. Will add that.

awesome

Incidentally, we really need to think about how we're organising our code. There is enough commonality between the different REST API (verification, provisioning, etc) implementations, such as the ReportProblem, that it would make sense to have a common place for the code, rather than duplicationg it for each API.

➕1

Raised #192

thomas-fossati · 2023-09-05T07:51:05Z

auth/keycloak.go

+	}
+
+	o.config = ginkeycloak.KeycloakConfig{
+		Url:                fmt.Sprintf("http://%s:%s", cfg.Host, cfg.Port),


It is quite security critical that the authoriser is authenticated.

(note to self: we need an issue for moving all the service endpoints to HTTPS.)

(note to self: we need an issue for moving all the service endpoints to HTTPS.)

Added #191

thomas-fossati · 2023-09-05T07:54:44Z

auth/keycloak.go

+
+	// note: this mapper function will only be called once the JWT had
+	// alreadybeen verified by ginkeycloak, so extracting claims without
+	// verification here is, in fact, safe.


paranoid mode on: have we checked that keycloak refuses JWTs with none algorithm?

haven't tried that, as the only way to easlily get a JWT than can be used for this is from keycloak it self. We can test it out at some point, but I would argue that is a security issue with keycloak.

I.e. it is somehting that needs to be tested and secured for a particular deployment, not withithin veraison code.

[...] but I would argue that is a security issue with keycloak.

yes

BTW, timely (but yet to be answered) question: keycloak/keycloak#22744

oh, yeah. Thanks for finding that. Upvoted and subscribed.

auth/keycloak.go

deployments/docker/src/veraison-realm.json

auth/basic.go

Implement API authentication and authorization. - Define IAuthorizer interface that can be used to obtain a gin middleware handler that performs authorisation. - Add a mechanism to obtain an IAuthorizer for a particular role base on Veraison configuration. - Implement "passthrough" authorizer that duplicates existing behavior (no auth). - Implement "basic" authorizer that does not rely on an external authorization server. - Implement "keycloak" authorizer that uses Keycloak service for authentication. - Update provisioning service to authorize based on "provisioner" role. - Update management service to authorize based on "manager" role. - Add the previously missing README for the management service. Signed-off-by: Sergei Trofimov <[email protected]>

- Update the deployment diagram to match current deployment. This includes diagramming the management service (was neglected when the service was added to the deployment). - Update REAME text around the diagram to match current deployment. - Add *.bkp to .gitignore. draw.io generates .bkp files when saving an updated diagram. Signed-off-by: Sergei Trofimov <[email protected]>

setrofim requested review from yogeshbdeshpande and thomas-fossati August 21, 2023 14:22

setrofim force-pushed the setrofim-auth branch from d8054cc to e7f8872 Compare August 24, 2023 15:16

setrofim mentioned this pull request Aug 29, 2023

Add authentication support veraison/apiclient#31

Merged