CLOSED - Oauth integration by mattjackson220 · Pull Request #3505 · apache/trafficcontrol

mattjackson220 · 2019-04-18T14:08:16Z

…rtal Login page updates, Traffic Ops API updates, and documentation updates

rebase got messed up. please reference new PR here:
#3763

What does this PR (Pull Request) do?

Sets up integration with OAuth providers to allow Single Sign On instead of username/password login to Traffic Portal. It defaults to disabled. Once OAuth is set up, login will follow these steps:

User will go to TP login page as usual, for example at tp.domain.com. If OAuth is enabled, a single button will be visible to login with SSO.
When button is clicked, it will redirect to the OAuth provider URL from the traffic_portal_properties.json.
OAuth provider will authenticate the user using SSO. If user is not logged in, they will be redirected to SSO login page. If user is logged in, they will be redirected to tp.domain.com/#!/sso?auth_token=encryptedTokenFromOAuthProvider
the /sso page will parse the token and POST to the API /user/login/oauth endpoint
the API /user/login/oauth endpoint will decode and validate the token, cross reference the Json Key Set URL against the whitelisted URLs in the cdn.conf file, query the database to get the user's role, and if all of that is successful, return a cookie.
If the login is successful, the user will be redirected to the page they were trying to see. If login was unsuccessful, a 401 error will be returned and the user will be redirected back to the login page.
This PR is not related to any Issue

Which Traffic Control components are affected by this PR?

Documentation
Traffic Ops
Traffic Portal

What is the best way to verify this PR?

To test using CDN In A Box:

Update config.sh to :
"whitelisted_oauth_urls": [
"insert domain for your expected Json Key Set returned by oAuth provider"
]
Update traffic_portal_properties.json to:
 "oAuth": {
"_comment": "Opt-in OAuth properties for SSO login",
"enabled": true,
"oAuthUrl": "insert your oAuth provider URL",
"oAuthTokenQueryParam": "insert your oAuth provider's token query parameter"
}
Run CDN in a box
Verify new login page only shows button to login with SSO
Click button with Network panel open and verify that it makes call to OAuth provider
Verify that login failed
Login to db container: 
docker exec -it cdn-in-a-box_db_1 /bin/bash
Login to postgres 
psql -d traffic_ops -U traffic_ops
Insert your user into db
 insert into tm_user (username, role, tenant_id) values ('yourUserId', (SELECT id FROM role WHERE name = 'admin'), (SELECT id FROM tenant WHERE name='root'));
Click Login button again, Verify call to OAuth provider, Verify that login succeeds
Log out
Try to go directly to /#!/servers endpoint
Verify it requires login and shows SSO login page
Click Login
Verify call to OAuth provider, verify login successful, verify it redirects to /#!/servers endpoint
Update config.sh to :
"whitelisted_oauth_urls": []
Restart Traffic Ops
Try to log in and verify it returns the following error:
"Key URL from token is not included in the whitelisted urls. Received: //url"

The following criteria are ALL met by this PR

This PR includes tests OR I have explained why tests are unnecessary
This PR includes documentation OR I have explained why documentation is unnecessary
This PR includes an update to CHANGELOG.md OR such an update is not necessary
This PR includes any and all required license headers
This PR ensures that database migration sequence is correct OR this PR does not include a database migration
This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

…rtal Login page updates, Traffic Ops API updates, and documentation updates

asfgit · 2019-04-18T14:11:06Z

Can one of the admins verify this patch?

…rtal Login page updates, Traffic Ops API updates, and documentation updates

ocket8888 · 2019-04-23T13:49:40Z

I'll check this out in a minute here

ocket8888

Your code-blocks in the quick-how-to section are indented with spaces instead of tabs.

Also, this is going to disable regular login when SSO is enabled, is that the intended behavior? Why not allow either when SSO is enabled, or maybe a separate flag for disabling regular login? In any case it won't prevent anyone from directly using the API to bypass SSO, so it's a bit lacking as a security measure - if that was the intention.

ocket8888 · 2019-04-23T16:11:53Z

 	ds_requests
 	federations
 	multi_site
+	oauth-login


this needs to reflect the document name, which isn't the same as the reference label at the top. That is, this should be oauth_login

ocket8888 · 2019-04-23T17:38:20Z

+*********************
+
+An opt-in configuration for SSO using OAuth is supported and can be configured through the :file:`/opt/traffic_portal/public/traffic_portal_properties.json` and :file:`/opt/traffic_ops/app/conf/cdn.conf` files.
+OAuth uses a third party provider to authenticate the user. Once enabled, the Traffic Portal Login page will no longer accept username and password but instead will authenticate using OAuth.


These line breaks are unnecessary/do nothing, and per the documentation guidelines shouldn't exist. The user-agent/output format will wrap the text appropriately.

ocket8888 · 2019-04-23T17:41:32Z

+An opt-in configuration for SSO using OAuth is supported and can be configured through the :file:`/opt/traffic_portal/public/traffic_portal_properties.json` and :file:`/opt/traffic_ops/app/conf/cdn.conf` files.
+OAuth uses a third party provider to authenticate the user. Once enabled, the Traffic Portal Login page will no longer accept username and password but instead will authenticate using OAuth.
+This will redirect to the ``oAuthUrl`` from :file:`/opt/traffic_portal/public/traffic_portal_properties.json` which will authenticate the user then redirect to the new ``/sso`` page with a Json Web Token added as a query parameter.
+The new ``/sso`` page will parse the token from the URL and post this information to the ``/api/1.4/user/login/oauth`` API endpoint. See :ref:`to-api-user-login-oauth`.


Instead of re-iterating the endpoint, you could just say

The new ``/sso`` page will parse the token from the URL and ``POST`` this information to the :ref:`to-api-user-login-oauth` API endpoint.

At any rate, please avoid using explicit API version numbers - it means that when API v1.5 or v2 is released we'll need to come back to this and clarify. Instead, by linking directly to the endpoint, readers can easily view the "New in" admonition which will inform them of the minimum API version needed to access the endpoint, regardless of whatever is the current version.

ocket8888 · 2019-04-23T17:42:01Z

+OAuth uses a third party provider to authenticate the user. Once enabled, the Traffic Portal Login page will no longer accept username and password but instead will authenticate using OAuth.
+This will redirect to the ``oAuthUrl`` from :file:`/opt/traffic_portal/public/traffic_portal_properties.json` which will authenticate the user then redirect to the new ``/sso`` page with a Json Web Token added as a query parameter.
+The new ``/sso`` page will parse the token from the URL and post this information to the ``/api/1.4/user/login/oauth`` API endpoint. See :ref:`to-api-user-login-oauth`.
+The ``/api/1.4/user/login/oauth`` API endpoint will decode the token, validate that it is between the issued time and the expiration time, and validate that the public key set URL is allowed by the list of whitelisted URLs read from :file:`/opt/traffic_ops/app/conf/cdn.conf`. It will then authorize the user from the database and return a mojolicious cookie as per the normal login workflow.


Same as above RE linking and explicit version numbers.

ocket8888 · 2019-04-23T17:45:46Z

+
+- Update :file:`/opt/traffic_portal/public/traffic_portal_properties.json` and ensure the following properties are set up correctly:
+
+.. table:: OAuth Configuration Property Definitions In traffic_portal_properties.json


this table should be indented by one level to properly place it as a part of the list item content. Currently it's rendering as something like:

<ul> <li>Update <code>/opt/traffic_portal/public/traffic_portal_properties.json</code> ...</i> </ul> <table>  </table> <ul> <li>The next item in the list</li> </ul>

ocket8888 · 2019-04-23T19:31:49Z

            <div class="x_title">
-                <h2>Login</h2>
+                <h2 ng-show="!oAuthEnabled">Login</h2>
+                <h2 ng-show="oAuthEnabled">Login With Single Sign On</h2>


instead of ng-show, consider ng-if since this should never change for a single pageload.

ocket8888 · 2019-04-23T19:34:07Z

            <div class="x_content">
                <br>
-                <form name="loginForm" role="form" class="form-horizontal form-label-left" novalidate>
+                <form name="loginForm" role="form" class="form-horizontal form-label-left" novalidate ng-show="!oAuthEnabled">


Same as above RE: ng-if vs ng-show.

Also, why are we doing novalidate on these forms? Your change is consistent with what we have already, but we aren't doing any angular validations on these forms, so novalidate only disables validations that aren't being overridden anyway.

done for the ng-if
as for the novalidate, is this worth updating now or should i stay with going for consistency with the other code?

unless you wanna change both - which you absolutely do not have to do -, don't bother

ocket8888 · 2019-04-23T19:35:34Z

                        </div>
                    </div>
                </form>
+                <form name="loginForm" role="form" class="form-horizontal form-label-left" novalidate ng-show="oAuthEnabled">


Same as above RE: ng-if vs ng-show

ocket8888 · 2019-04-23T19:36:30Z

+
+        authService.oauthLogin(token)
+            .then(
+                function() {}


If you're not doing anything, do you need a .then? Maybe just log a success message or something?

ocket8888 · 2019-04-23T19:37:06Z

+under the License.
+-->
+
+<div id="ssoContainer"/>


Same as above RE: terminating newlines

also, as far as I can tell, this is never styled or accessed anywhere, so could it just have a blank template instead of this file? Or maybe I'm wrong and Angular will complain about blank templates. In that case, I still don't think this id is ever used for anything.

Also, according to the HTML spec the DIV element may not omit a closing tag.

done. you were right, i didnt need it at all and could just use a blank template.

ocket8888 · 2019-04-23T19:43:29Z

+********************
+``user/login/oauth``
+********************
+


You should use a .. versionadded:: directive to clarify in what API version(s) this endpoint is available.

mitchell852 · 2019-04-29T14:40:44Z

@mattjackson220 - since you created a new api endpoint. can you create a new "capability" called sso_auth or oath or whatever you think is best and and add it to every role in seeds.sql and patches.sql? let me know if you have questions about that.

ocket8888 · 2019-05-02T18:35:09Z

+
+- Update :file:`/opt/traffic_portal/public/traffic_portal_properties.json` and ensure the following properties are set up correctly:
+
+        .. table:: OAuth Configuration Property Definitions In traffic_portal_properties.json


looks like you're using spaces for indentation - we tabs now.

ocket8888 · 2019-05-02T18:37:27Z

+
+``POST``
+========
+.. versionadded:: 1.4


This should go at the endpoint level, not the method level - it should be right under the page title. If for some reason we added a GET method to this endpoint - which we'd probably never do, but bear with me on this - it'll look like that has always existed and POST was added in 1.4.

mitchell852 · 2019-05-08T02:15:55Z

+    "oAuth": {
+      "_comment": "Opt-in OAuth properties for SSO login",
+      "enabled": false,
+      "oAuthUrl": "",


can you put some kind of fake example value in oAuthUrl and oAuthTokenQueryParam just to give us an idea of what to put in here. also, can you maybe explain a bit more in the _comment field? i'm guessing that with enabled=false, those values get ignored anyhow, right? url is pretty easy to figure out but i'm guessing for oAuthTokenQueryParam just a name will suffice?

mitchell852

looking good from a TP perspective. added a few comments.

mitchell852 · 2019-05-08T02:46:41Z

+var SsoController = function($scope, $state, $interval, authService, propertiesModel) {
+
+	var init = function () {
+	    const currentUrl = new URL(window.location.href);


i'm a bit confused about what's going on here. I "think" you're trying to get the value of the "oAuthTokenQueryParam" query param. so if oAuthTokenQueryParam=token, you're trying to get foo from ?token=foo.

if so, you could have done something like this i think:

let tokenParameter = propertiesModel.properties.oAuth.oAuthTokenQueryParam let tokenValue = $location.search()[tokenParameter]; authService.oauthLogin(tokenValue);

mitchell852 · 2019-05-08T03:02:27Z

-var LoginController = function($scope, $log, $uibModal, authService, userService) {
+var LoginController = function($scope, $log, $uibModal, authService, userService, propertiesModel) {
+
+    $scope.returnUrl = window.location.hostname;


I don't see where returnUrl, returnPort or returnProtocol are being used

mitchell852 · 2019-05-08T03:09:36Z

+        var continueParam = new URL(window.location.href);
+        continueParam.hash = '#!/sso?redirect=' + redirectParam;
+
+        var continueURL = new URL(gotoUrl.protocol + '//' + gotoUrl.hostname + gotoUrl.pathname);


this is all pretty confusing. can you add some comments to explain the format of the url you are building to send them to?

mitchell852 · 2019-05-08T03:11:29Z

                        </div>
                    </div>
                </form>
+                <form name="loginForm" role="form" class="form-horizontal form-label-left" novalidate ng-if="oAuthEnabled">


can you rename this to name="ssoLoginForm" or something so there's not 2 forms of the same name?

mitchell852 · 2019-05-08T03:14:00Z

+        return httpService.post(ENV.api['root'] + 'user/login/oauth', { t: token})
+            .then(
+                function(result) {
+                    $rootScope.$broadcast('authService::oauthLogin');


you can change this to

$rootScope.$broadcast('authService::login');

if you do a search for authService::login you'll see what dispatching that event does.

mitchell852 · 2019-05-08T03:18:07Z

+                <form name="loginForm" role="form" class="form-horizontal form-label-left" novalidate ng-if="oAuthEnabled">
+                    <div class="form-group">
+                        <div class="col-md-6 col-sm-6 col-xs-12 col-md-offset-1">
+                            <button name="loginOauthSubmit" type="submit" class="btn btn-primary" ng-click="loginOauth()">Log in &nbsp;&nbsp;<i class="fa fa-chevron-circle-right"></i></button>


i think this should be changed to type=button

Either that or ng-click on the button could be ng-submit on the form

mitchell852 · 2019-05-08T03:22:44Z

+                        $location.url('/');
+                    }
+                },
+                function(fault) {


any failure will be a 401, right? i.e. if it turns out that the token is bogus or username doesn't exist in the database is a 401 returned? if so, you can kill this 2nd function as 401s are captured globally

mitchell852 · 2019-05-10T18:42:54Z

That button looks funny. Can you kill the col-md-offset-1

# Conflicts: # CHANGELOG.md

mitchell852 · 2019-05-14T15:01:28Z

      ]
+    },
+    "oAuth": {
+      "_comment": "Opt-in OAuth properties for SSO login.  oAuthUrl is the url to redirect to for your oAuth provider.  oAuthTokenQueryParam is the query parameter key that the oAuth provider will use for the token (defaults to access_token).  redirectUriParameterOverride is to override if the oAuth provider requires a different key for the redirect_uri parameter (defaults to redirect_uri if empty). clientId is the client to be sent to the oAuth provider in the client_id field.",


Now that i think about it more, maybe it makes more sense to just have a link to http://traffic-control-cdn.readthedocs.io/en/latest/admin/quick_howto/oauth_login.html in the _comment field. what do you think?

i like that! ill update it. i might keep the defaults in there too so that its a quick and easy thing to reference

In general linking to the latest docs is dangerous - someone who deploys 4.0 can get directed to the 5.0 docs.

good call. done

mitchell852 · 2019-05-14T15:09:15Z

            "mojolicious": 4
        },
-	"profiling_enabled": false
+        "whitelisted_oauth_urls": [],


i can't decide if this should go inside of traffic_ops_golang or outside of it. maybe @dangogh has some ideas. not sure if the traffic_ops_golang node was reserved for certain properties or if they are arbitrary.

I think if it's something that's directly related to the execution of traffic_ops_golang, it should be inside. This looks like something outside of that. But, scanning the list, it doesn't look like that was adhered to, so that's not a strong opinion....

mitchell852 · 2019-05-14T15:24:20Z

If i forget to put the oauth url in the whitelisted_oauth_urls, i get a 500 ISE:

can you kick back a 401 instead?

mitchell852 · 2019-05-14T15:42:26Z

 - update an existing user
 - view :term:`Delivery Service`\ s visible to a user

+.. Note:: If OAuth is enabled, creating/updating a user here will update the user's roles but the user needs to be created/updated with the OAuth provider as well.


a user just has one role

Also, Role is a term

done. also changing 'updating' to 'deleting' since right now oauth only deals with user authentication and changing the user's role will not have any effect on it, so only creates/deletes need to be done in the oauth provider too

ocket8888

Mini-review for docs

ocket8888 · 2019-05-14T20:33:34Z

+
+- Update :file:`/opt/traffic_portal/public/traffic_portal_properties.json` and ensure the following properties are set up correctly:
+
+        .. table:: OAuth Configuration Property Definitions In traffic_portal_properties.json


This still appears to be spaces... check your editor setting, it might be changing them automatically on save.

dang it thats exactly what's happening. i think i got it right this time. sorry about that

ocket8888 · 2019-05-14T20:34:35Z

 - update an existing user
 - view :term:`Delivery Service`\ s visible to a user

+.. Note:: If OAuth is enabled, creating/updating a user here will update the user's roles but the user needs to be created/updated with the OAuth provider as well.


Also, Role is a term

…plicit oauth workflow

…rtal Login page updates, Traffic Ops API updates, and documentation updates

…plicit oauth workflow

…/trafficcontrol into oauth_integration

asfgit · 2019-07-30T18:52:13Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4056/
Test FAILed.

mattjackson220 · 2019-07-31T19:19:15Z

Rebase got messed up so here is a new version of this PR:
#3763

mattjackson220 added 2 commits April 17, 2019 14:52

Setup for Opt-In Integration with OAuth provider including Traffic Po…

7874d0b

…rtal Login page updates, Traffic Ops API updates, and documentation updates

Updated changelog.md and added test

a324ba4

mattjackson220 added 3 commits April 18, 2019 08:12

Setup for Opt-In Integration with OAuth provider including Traffic Po…

0a0c631

…rtal Login page updates, Traffic Ops API updates, and documentation updates

Updated changelog.md and added test

5037c8d

fixed merge conflict

bfebf50

dangogh added documentation related to documentation new feature A new feature, capability or behavior Traffic Ops related to Traffic Ops Traffic Portal v1 related to Traffic Portal version 1 labels Apr 18, 2019

minor format fixes

26bb05d

ocket8888 suggested changes Apr 23, 2019

View reviewed changes

mattjackson220 added 2 commits May 2, 2019 11:30

Updates per comments on PR

294af5a

Updates per comments on PR

2385de4

ocket8888 suggested changes May 2, 2019

View reviewed changes

mitchell852 reviewed May 8, 2019

View reviewed changes

mattjackson220 added 2 commits May 8, 2019 14:59

updates per comments

112072c

added /sso redirect to root app to comply with OAuth standards

577f420

mattjackson220 added 2 commits May 10, 2019 15:36

Merge branch 'master' into oauth_integration

c12ac9d

# Conflicts: # CHANGELOG.md

updates per comments

95fb2b0

mitchell852 reviewed May 14, 2019

View reviewed changes

ocket8888 suggested changes May 14, 2019

View reviewed changes

mattjackson220 added 24 commits July 30, 2019 07:10

Updates per comments on PR

c867470

updates per comments

480df84

added /sso redirect to root app to comply with OAuth standards

e4bb18e

updates per comments

6c9c546

updates per comments

6a52ac8

updated to use authorization code instead of token, getting rid of im…

e955b1f

…plicit oauth workflow

updated so code is in POST body

dc76109

updated so all parameters are in POST body

831963d

updated to parse token from json or as response body

b453c2a

updated per comments

97ab31b

update per comment

2814223

update per comments

1345261

vendored dgrijalva/jwt-go and lestrrat-go/jwx

4a68fe0

update for weasel with new dependencies

2e1834c

update per comment

9340f27

update per comment

b5396e6

Setup for Opt-In Integration with OAuth provider including Traffic Po…

98759f7

…rtal Login page updates, Traffic Ops API updates, and documentation updates

Updated changelog.md and added test

96a1a6f

Updated changelog.md and added test

086ad68

Updates per comments on PR

35cbe39

updated to use authorization code instead of token, getting rid of im…

14d3816

…plicit oauth workflow

update per comment

7a3ea65

update per comments

0346476

Merge branch 'oauth_integration' of https://github.com/mattjackson220…

066bda0

…/trafficcontrol into oauth_integration

mattjackson220 closed this Jul 31, 2019

mitchell852 changed the title ~~Oauth integration~~ CLOSED - Oauth integration Oct 24, 2019

mitchell852 added the abandoned label Sep 2, 2020

zrhoffman added the authentication Relating to login, registration, passwords, tokens, etc. label Jan 12, 2022


		- Update :file:`/opt/traffic_portal/public/traffic_portal_properties.json` and ensure the following properties are set up correctly:

		.. table:: OAuth Configuration Property Definitions In traffic_portal_properties.json

Conversation

mattjackson220 commented Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR (Pull Request) do?

Which Traffic Control components are affected by this PR?

What is the best way to verify this PR?

The following criteria are ALL met by this PR

Uh oh!

asfgit commented Apr 18, 2019

Uh oh!

ocket8888 commented Apr 23, 2019

Uh oh!

ocket8888 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitchell852 commented Apr 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitchell852 May 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitchell852 left a comment

mattjackson220 commented Apr 18, 2019 •

edited

Loading

mitchell852 May 8, 2019 •

edited

Loading

mitchell852 May 8, 2019 •

edited

Loading