Add securityDefinitions to the generated OpenAPI spec #14745

simo5 · 2017-06-19T13:38:39Z

enj

Add OAuth2 bits, remove x509 I guess...

enj · 2017-06-19T13:50:24Z

pkg/cmd/server/kubernetes/master/master_config.go

+ }
+ }
+ if configapi.UseTLS(config.ServingInfo.ServingInfo) {
+ // TODO: How is this represented ?


So based on http://swagger.io/specification/#securityDefinitionsObject I would have to say there is no way to assert our use of x509 😞

Not supported indeed:
OAI/OpenAPI-Specification#1004

enj · 2017-06-19T13:56:18Z

pkg/cmd/server/kubernetes/master/master_config.go

@@ -642,7 +642,22 @@ func BuildKubernetesMasterConfig(
 return kmaster, nil
 }

-func DefaultOpenAPIConfig() *openapicommon.Config {
+func DefaultOpenAPIConfig(config configapi.MasterConfig) *openapicommon.Config {
+ securityDefinitions := spec.SecurityDefinitions{}


Based on http://swagger.io/specification/#securityDefinitionsObject we should include OAuth2 info here using logic similar to https://github.com/openshift/origin/blob/master/pkg/cmd/server/origin/nonapiserver.go#L91-L93

I am not sure where to source the right info, for example about flow type and scopes, and no other code in the origin repo uses the oauth2 type yet, it seem any outh based on any kind of token is defined just as an apikey bearer token...

enj · 2017-06-20T16:47:55Z

pkg/cmd/server/kubernetes/master/master_config.go

+ SecuritySchemeProps: spec.SecuritySchemeProps{
+ Type: "oauth2",
+ Flow: "accessCode",
+ AuthorizationURL: config.OAuthConfig.MasterURL + "/oauth/authorize",


Not sure I want to use config.OAuthConfig.MasterURL here because then it is easy for generated code to not line up between local env and Jenkins.

what should we use ?
Initially I have MasterPublicURL, but it seems you object to use these variables completely ?

Something like this would work (hopefully there is not an import cycle):

... AuthorizationURL: OpenShiftOAuthAuthorizeURL("https://localhost:8443"), TokenURL: OpenShiftOAuthTokenURL("https://localhost:8443"), ...

Actually I thought about this some more and we should be able to use the variables. We just need to make sure that the Swagger generation script always starts a server on localhost.

Why the script should start a server ?

We end up in an import cycle if I try to import pkg/cmd/server/origin in order to access auth.OpenShiftOAuthAuthorizeURL ...

Why the script should start a server ?

hack/update-generated-swagger-spec.sh must build the swagger spec from a running server (there is no other way to get its final runtime state). The script already uses 127.0.0.1 so you are good to use the variables.

enj · 2017-06-20T16:51:24Z

pkg/cmd/server/kubernetes/master/master_config.go

@@ -654,6 +654,16 @@ func DefaultOpenAPIConfig(config configapi.MasterConfig) *openapicommon.Config {
 },
 }
 }
+ if config.OAuthConfig != nil {
+ securityDefinitions["Oauth2AccessToken"] = &spec.SecurityScheme{


I would expect to see all flows from #10845 (comment):

root@test:/# curl -k https://openshift.default.svc/.well-known/oauth-authorization-server { "issuer": "https://10.13.129.56:8443", "authorization_endpoint": "https://10.13.129.56:8443/oauth/authorize", "token_endpoint": "https://10.13.129.56:8443/oauth/token", "scopes_supported": [ "user:full", "user:info", "user:check-access", "user:list-scoped-projects", "user:list-projects" ], "response_types_supported": [ "code", "token" ], "grant_types_supported": [ "authorization_code", "implicit" ], "code_challenge_methods_supported": [ "plain", "S256" ] }

I do not see any flows defined in that PR ?

implicit and accessCode (each with the scopes and URLs).

enj · 2017-06-21T14:24:10Z

pkg/cmd/util/oauth.go

@@ -0,0 +1,22 @@
+package util


Move to pkg/oauth/util/util.go or maybe combine with pkg/oauth/discovery/discovery.go.

enj · 2017-06-21T14:25:01Z

pkg/cmd/server/kubernetes/master/master_config.go

+ }
+ if configapi.UseTLS(config.ServingInfo.ServingInfo) {
+ // No support in Swagger's OpenAPI sepc v.2 ¯\_(ツ)_/¯
+ // TODO: Add x509 specification once available


We should have a separate GitHub issue to track this.

@simo5 tag the issue you opened here.

enj · 2017-06-21T14:26:59Z

pkg/authorization/authorizer/scope/converter.go

@@ -135,6 +135,16 @@ const (
 UserFull = UserIndicator + "full"
 )

+func DefaultSupportedScopes() []string {
+ return []string{


Make this list a private var and have this function return it (convention we use everywhere).

enj · 2017-06-21T14:37:30Z

pkg/cmd/server/kubernetes/master/master_config.go

+ for _, s := range scope.DefaultSupportedScopes() {
+ defScopes[s] = ""
+ }
+ securityDefinitions["Oauth2Immediate"] = &spec.SecurityScheme{


Where did Immediate come from?

Word salads from my brain

enj · 2017-06-21T15:31:29Z

pkg/authorization/authorizer/scope/converter.go

+ UserFull: `Full read/write access with all of your permissions`,
+}
+
+func DefaultSupportedScopes() []string {


We need this to have a consistent order so use return sets.StringKeySet(defaultSupportedScopesMap).List() (you can store this as a variable if you want to).

sets = "k8s.io/apimachinery/pkg/util/sets"

enj · 2017-06-21T15:34:03Z

pkg/authorization/authorizer/scope/converter.go

@@ -154,15 +176,15 @@ func (userEvaluator) Validate(scope string) error {
 func (userEvaluator) Describe(scope string) (string, string, error) {
 switch scope {
 case UserInfo:
- return "Read-only access to your user information (including username, identities, and group membership)", "", nil
+ return defaultSupportedScopesMap[UserInfo], "", nil


Collapse the first four cases since they are the same and use defaultSupportedScopesMap[scope] for all cases.

pushed a commit for this

enj · 2017-06-21T15:38:33Z

pkg/oauth/discovery/discovery.go

- scope.UserListAllProjects,
- },
+ // Note: this list is incomplete, which is allowed per the draft spec
+ ScopesSupported: scope.DefaultSupportedScopes(),


You will have to update the unit test if the order of the scopes changes.

and another for this

enj · 2017-06-21T15:45:02Z

api/swagger-spec/openshift-openapi-spec.json

+ }
+ }
+ },
+ "security": [


I am curious what this is trying to convey...

simo5 · 2017-06-21T16:39:09Z

combined pkg/oauth/util/util.go and pkg/oauth/discovery/discovery.go under a new pkg/oauth/util including a url.go and a discovery.go files

enj · 2017-06-21T16:44:51Z

pkg/authorization/authorizer/scope/converter.go

@@ -154,15 +177,15 @@ func (userEvaluator) Validate(scope string) error {
 func (userEvaluator) Describe(scope string) (string, string, error) {
 switch scope {
 case UserInfo:
- return "Read-only access to your user information (including username, identities, and group membership)", "", nil
+ fallthrough


case UserFull, UserAccessCheck, ...: (no fallthrough please)

enj · 2017-06-21T16:45:00Z

pkg/authorization/authorizer/scope/converter.go

+ return sets.StringKeySet(defaultSupportedScopesMap).List()
+ // scopes := make([]string, len(defaultSupportedScopesMap))
+ // i := 0
+ // for k := range defaultSupportedScopesMap {


enj · 2017-06-21T16:45:36Z

pkg/oauth/util/discovery.go

@@ -38,19 +38,14 @@ type OauthAuthorizationServerMetadata struct {
 CodeChallengeMethodsSupported []string `json:"code_challenge_methods_supported"`
 }

-func Get(masterPublicURL, authorizeURL, tokenURL string) OauthAuthorizationServerMetadata {
+func GetOauthMetadata(masterPublicURL, authorizeURL, tokenURL string) OauthAuthorizationServerMetadata {


Change to just take masterPublicURL and use the helpers inside.

enj · 2017-06-21T16:45:59Z

pkg/cmd/server/origin/nonapiserver.go

@@ -11,7 +11,7 @@ import (

 "github.com/openshift/origin/pkg/cmd/server/bootstrappolicy"
 "github.com/openshift/origin/pkg/cmd/util/plug"
- "github.com/openshift/origin/pkg/oauth/discovery"
+ "github.com/openshift/origin/pkg/oauth/util"


Call it oauthutil like the rest of the imports.

simo5 · 2017-06-21T17:07:58Z

With this latest push/rebase I thikn I have addressed all the concerns.

enj · 2017-06-21T17:26:16Z

[test]

enj · 2017-06-21T17:35:55Z

pkg/oauth/util/discovery_test.go

@@ -8,7 +8,7 @@ import (
 )

 func TestGet(t *testing.T) {
- actual := Get("https://localhost:8443", "https://localhost:8443/oauth/authorize", "https://localhost:8443/oauth/token")
+ actual := GetOauthMetadata("https://localhost:8443")


Capitalize the A in OAuth: GetOauthMetadata -> GetOAuthMetadata (sorry I missed this earlier)

nvm, this is consistent with the containing file and the whole code base is really inconsistent on this anyway.

enj · 2017-06-21T20:36:47Z

@simo5 please fix the failing test: github.com/openshift/origin/pkg/oauth/util.TestGet

enj · 2017-06-22T12:14:24Z

cc @liggitt in case you want to take a look as well.

enj · 2017-06-23T00:33:13Z

@caseyfw are you able to test this?

cc @openshift/security

enj · 2017-06-26T21:26:41Z

@simo5 master is open again so I can tag this if you fix the conflicts.

simo5 · 2017-06-27T08:56:11Z

Rebased

enj · 2017-06-28T00:21:34Z

@simo5 you will have to rebase on top of master again and then regenerate again since the spec changed for other reasons.

Also add helper function to return scope:description map used in a later commit Signed-off-by: Simo Sorce <simo@redhat.com>

In preparation of future patches where we need to access these helpers without causing an import cycle Signed-off-by: Simo Sorce <simo@redhat.com>

Signed-off-by: Simo Sorce <simo@redhat.com>

simo5 · 2017-06-28T11:19:58Z

Rebased once again, let's hope this time it will stick

simo5 · 2017-06-28T11:21:46Z

FWIW: for some reason I had to git clean -f -x -d -f and then make again, only then the swagger update script generated some changes ...

openshift-bot · 2017-06-28T11:23:38Z

Evaluated for origin test up to b088faa

enj · 2017-06-28T13:14:56Z

[merge] failures are unrelated flakes.

@simo5 please tag the flakes so we can track them.

openshift-bot · 2017-06-28T13:15:49Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2746/) (Base Commit: 6bbad88) (PR Branch Commit: b088faa)

simo5 · 2017-06-28T15:16:37Z

flaked on #14878

simo5 · 2017-06-28T15:17:18Z

flaked on #14043

simo5 · 2017-06-30T12:19:00Z

merge flaked on #14897

enj · 2017-06-30T14:44:55Z

re[merge] flaked on #14897

openshift-bot · 2017-06-30T14:47:34Z

Evaluated for origin merge up to b088faa

openshift-bot · 2017-06-30T19:55:36Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1189/) (Base Commit: 7004e83) (PR Branch Commit: b088faa) (Image: devenv-rhel7_6416)

enj · 2017-06-30T22:26:52Z

@simo5 congrats \o/

deads2k · 2017-07-03T13:03:22Z

pkg/cmd/server/origin/asset.go

@@ -27,6 +27,7 @@ import (
 configapi "github.com/openshift/origin/pkg/cmd/server/api"
 "github.com/openshift/origin/pkg/cmd/server/crypto"
 cmdutil "github.com/openshift/origin/pkg/cmd/util"
+ oauthutil "github.com/openshift/origin/pkg/oauth/util"


This package dependency was previously excluded intentionally. The asset server should not depend on the oauth package at all. The two are logically independent.

ioggstream · 2019-02-11T10:55:31Z

FYI @simo5 we just merged mutualTLS in OAS3.1. Still we didn't add further specs as we didn't find an agreement on the description model. A proposal could be very interesting though.

simo5 · 2019-02-11T13:36:05Z

@mrogers950 or @enj can you take a look ?

simo5 assigned enj and simo5 Jun 19, 2017

simo5 requested a review from enj June 19, 2017 13:38

enj suggested changes Jun 19, 2017

View reviewed changes

simo5 added the area/security label Jun 19, 2017

enj suggested changes Jun 20, 2017

View reviewed changes

simo5 force-pushed the issue/14268 branch from 0c9451e to b6799cd Compare June 21, 2017 14:07

enj suggested changes Jun 21, 2017

View reviewed changes

simo5 force-pushed the issue/14268 branch from c16955b to 435c88a Compare June 21, 2017 15:56

enj suggested changes Jun 21, 2017

View reviewed changes

simo5 force-pushed the issue/14268 branch 2 times, most recently from 4260fd6 to 266ab70 Compare June 21, 2017 17:07

enj reviewed Jun 21, 2017

View reviewed changes

enj changed the title ~~[WIP] Add securityDefinitions to the generated OpenAPI spec~~ Add securityDefinitions to the generated OpenAPI spec Jun 21, 2017

enj unassigned simo5 Jun 21, 2017

enj approved these changes Jun 21, 2017

View reviewed changes

enj mentioned this pull request Jun 21, 2017

Fix API to Add x509 securityDefinition once OpenAPI support it #14811

Open

simo5 force-pushed the issue/14268 branch 2 times, most recently from dc07994 to 8a01a78 Compare June 22, 2017 17:34

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2017

simo5 force-pushed the issue/14268 branch from 8a01a78 to 436b371 Compare June 27, 2017 08:55

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 27, 2017

simo5 force-pushed the issue/14268 branch from 436b371 to 0f68ff5 Compare June 27, 2017 19:49

enj removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2017

simo5 added 4 commits June 28, 2017 07:10

Use function to define default scopes

632c796

Also add helper function to return scope:description map used in a later commit Signed-off-by: Simo Sorce <simo@redhat.com>

Move some oauth helpers in a new util package

947758b

In preparation of future patches where we need to access these helpers without causing an import cycle Signed-off-by: Simo Sorce <simo@redhat.com>

Add securityDefinitions to the generated OpenAPI spec

9f8e277

Signed-off-by: Simo Sorce <simo@redhat.com>

Autogenerated Swagger Spec

b088faa

Signed-off-by: Simo Sorce <simo@redhat.com>

simo5 force-pushed the issue/14268 branch from 0f68ff5 to b088faa Compare June 28, 2017 11:19

openshift-bot merged commit 848f52e into openshift:master Jun 30, 2017

deads2k reviewed Jul 3, 2017

View reviewed changes

ioggstream mentioned this pull request May 17, 2018

[OpenAPI.Next Proposal] Add client certificate to authentication methods OAI/OpenAPI-Specification#1004

Closed

Add securityDefinitions to the generated OpenAPI spec #14745

Add securityDefinitions to the generated OpenAPI spec #14745

Conversation

simo5 commented Jun 19, 2017 • edited by enj Loading

enj 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

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

simo5 commented Jun 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enj Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

simo5 commented Jun 21, 2017

enj commented Jun 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enj commented Jun 21, 2017

enj commented Jun 22, 2017

enj commented Jun 23, 2017

enj commented Jun 26, 2017

simo5 commented Jun 27, 2017

enj commented Jun 28, 2017

simo5 commented Jun 28, 2017

simo5 commented Jun 28, 2017

openshift-bot commented Jun 28, 2017

enj commented Jun 28, 2017

openshift-bot commented Jun 28, 2017

simo5 commented Jun 28, 2017

simo5 commented Jun 28, 2017

simo5 commented Jun 30, 2017

enj commented Jun 30, 2017

openshift-bot commented Jun 30, 2017

openshift-bot commented Jun 30, 2017 • edited Loading

enj commented Jun 30, 2017

Choose a reason for hiding this comment

ioggstream commented Feb 11, 2019

simo5 commented Feb 11, 2019

simo5 commented Jun 19, 2017 •

edited by enj

Loading

enj Jun 21, 2017 •

edited

Loading

openshift-bot commented Jun 30, 2017 •

edited

Loading