You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by "kdamichie (via GitHub)" <gi...@apache.org> on 2023/07/07 15:56:28 UTC
[GitHub] [trafficcontrol] kdamichie opened a new pull request, #7619: Oauth go dependency change
kdamichie opened a new pull request, #7619:
URL: https://github.com/apache/trafficcontrol/pull/7619
<!--
Thank you for contributing! Please be sure to read our contribution guidelines: https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md
If this closes or relates to an existing issue, please reference it using one of the following:
Closes: #ISSUE
Related: #ISSUE
If this PR fixes a security vulnerability, DO NOT submit! Instead, contact
the Apache Traffic Control Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://apache.org/security regarding vulnerability disclosure.
-->
This PR Addresses a needed field to be provided for OAuth security when using Microsoft Authenticator
<!-- **^ Add meaningful description above** --><hr/>
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this PR.
Feel free to add the name of a tool or script that is affected but not on the list.
-->
- Documentation
- Traffic Ops
## What is the best way to verify this PR?
<!-- Please include here ALL the steps necessary to test your PR.
If your PR has tests (and most should), provide the steps needed to run the tests.
If not, please provide step-by-step instructions to test the PR manually and explain why your PR does not need tests. -->
Standup a local instance of CIAB, and redirect your localhost to nightly URL. Ensure your user credentials are in the DB. Next log into TP using SSO option. This should log you in without error.
## If this is a bugfix, which Traffic Control versions contained the bug?
<!-- Delete this section if the PR is not a bugfix, or if the bug is only in the master branch.
Examples:
- 5.1.2
- 5.1.3 (RC1)
-->
## PR submission checklist
- [ ] This PR has tests <!-- If not, please delete this text and explain why this PR does not need tests. -->
- [x] This PR has documentation <!-- If not, please delete this text and explain why this PR does not need documentation. -->
- [x] This PR has a CHANGELOG.md entry <!-- A fix for a bug from an ATC release, an improvement, or a new feature should have a changelog entry. -->
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://apache.org/security) for details)
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a diff in pull request #7619: Oauth go dependency change
Posted by "srijeet0406 (via GitHub)" <gi...@apache.org>.
srijeet0406 commented on code in PR #7619:
URL: https://github.com/apache/trafficcontrol/pull/7619#discussion_r1256438286
##########
docs/source/admin/traffic_ops.rst:
##########
@@ -428,6 +428,7 @@ This file deals with the configuration parameters of running Traffic Ops itself.
:log_location_warning: This optional field, if specified, should either be the location of a file to which warning-level output will be logged, or one of the special strings ``"stdout"`` which indicates that STDOUT should be used, ``"stderr"`` which indicates that STDERR should be used or ``"null"`` which indicates that no output of this level should be generated. An empty string (``""``) and literally ``null`` are equivalent to ``"null"``. Default if not specified is ``"null"``.
:max_db_connections: An optional limit on the number of allowed concurrent connections to the Traffic Ops Database. If it is less than or equal to zero, there is no limit. Default if not specified is zero.
:oauth_client_secret: An optional secret string to be shared with OAuth-capable clients attempting to authenticate via OAuth. The default behavior if this is not defined - or is an empty string (``""``) or ``null`` is to disallow authentication via OAuth.
+ :oauth_user_attribute: An optional username string to be shared with OAuth-capable clients attempting to authenticate via OAuth. The default behavior if this is not defined - or is an empty string (``""``) or ``null`` is to disallow authentication via OAuth.
Review Comment:
The `-` should probably be in ````
##########
traffic_ops/traffic_ops_golang/login/login.go:
##########
@@ -483,17 +483,29 @@ func OauthLoginHandler(db *sqlx.DB, cfg config.Config) http.HandlerFunc {
return
}
- decodedToken, err := jwt.Parse(
+ var decodedToken jwt.Token
+ if decodedToken, err = jwt.Parse(
[]byte(encodedToken),
jwt.WithVerifyAuto(true),
jwt.WithJWKSetFetcher(jwksFetcher),
- )
- if err != nil {
- api.HandleErr(w, r, nil, http.StatusInternalServerError, nil, fmt.Errorf("Error decoding token with message: %w", err))
- return
+ ); err != nil {
+ if decodedToken, err = jwt.Parse(
+ []byte(encodedToken),
+ jwt.WithVerifyAuto(false),
+ jwt.WithJWKSetFetcher(jwksFetcher),
+ ); err != nil {
+ api.HandleErr(w, r, nil, http.StatusInternalServerError, nil, fmt.Errorf("Error decoding token with message: %w", err))
Review Comment:
I know this is not your code, but while you're in here, mind fixing the error statement to start with a lower case letter?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a diff in pull request #7619: Oauth go dependency change
Posted by "zrhoffman (via GitHub)" <gi...@apache.org>.
zrhoffman commented on code in PR #7619:
URL: https://github.com/apache/trafficcontrol/pull/7619#discussion_r1256501311
##########
dev/traffic_ops/cdn.json:
##########
@@ -10,6 +10,7 @@
"log_location_event": "stdout",
"whitelisted_oauth_urls": [],
"oauth_client_secret": "",
+ "oauth_user_attribute": "unique_name",
Review Comment:
>> Would these also need to be added to the ansible main.yml file?
>>
> @zrhoffman do we need it there?
The Traffic Ops Ansible role doesn't yet have OAuth stuff on the ATC master branch. @dsouza93 is adding it in [`dsouza93:lab_oath](https://github.com/dsouza93/trafficcontrol/commits/lab_oath) (but has not PRed it yet).
So, adding it in #7619 is not really possible without creating conflicts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 merged pull request #7619: Oauth go dependency change
Posted by "srijeet0406 (via GitHub)" <gi...@apache.org>.
srijeet0406 merged PR #7619:
URL: https://github.com/apache/trafficcontrol/pull/7619
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] codecov[bot] commented on pull request #7619: Oauth go dependency change
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #7619:
URL: https://github.com/apache/trafficcontrol/pull/7619#issuecomment-1625645604
## [Codecov](https://app.codecov.io/gh/apache/trafficcontrol/pull/7619?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#7619](https://app.codecov.io/gh/apache/trafficcontrol/pull/7619?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (d148da9) into [master](https://app.codecov.io/gh/apache/trafficcontrol/commit/8a08e3b187de43bd78188feda005dc75a445fd6d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (8a08e3b) will **decrease** coverage by `0.01%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #7619 +/- ##
============================================
- Coverage 30.13% 30.13% -0.01%
Complexity 98 98
============================================
Files 794 794
Lines 84095 84106 +11
Branches 907 907
============================================
+ Hits 25344 25345 +1
- Misses 56622 56633 +11
+ Partials 2129 2128 -1
```
| Flag | Coverage Δ | |
|---|---|---|
| golib_unit | `48.30% <ø> (ø)` | |
| grove_unit | `4.60% <ø> (ø)` | |
| t3c_unit | `5.28% <ø> (ø)` | |
| traffic_monitor_unit | `21.30% <ø> (ø)` | |
| traffic_ops_unit | `22.97% <0.00%> (-0.01%)` | :arrow_down: |
| traffic_portal_v2 | `73.78% <ø> (+0.02%)` | :arrow_up: |
| traffic_stats_unit | `10.14% <ø> (ø)` | |
| unit_tests | `27.26% <0.00%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/trafficcontrol/pull/7619?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [traffic\_ops/traffic\_ops\_golang/config/config.go](https://app.codecov.io/gh/apache/trafficcontrol/pull/7619?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-dHJhZmZpY19vcHMvdHJhZmZpY19vcHNfZ29sYW5nL2NvbmZpZy9jb25maWcuZ28=) | `36.20% <ø> (ø)` | |
| [traffic\_ops/traffic\_ops\_golang/login/login.go](https://app.codecov.io/gh/apache/trafficcontrol/pull/7619?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-dHJhZmZpY19vcHMvdHJhZmZpY19vcHNfZ29sYW5nL2xvZ2luL2xvZ2luLmdv) | `10.99% <0.00%> (-0.26%)` | :arrow_down: |
... and [1 file with indirect coverage changes](https://app.codecov.io/gh/apache/trafficcontrol/pull/7619/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] kdamichie commented on a diff in pull request #7619: Oauth go dependency change
Posted by "kdamichie (via GitHub)" <gi...@apache.org>.
kdamichie commented on code in PR #7619:
URL: https://github.com/apache/trafficcontrol/pull/7619#discussion_r1256478786
##########
dev/traffic_ops/cdn.json:
##########
@@ -10,6 +10,7 @@
"log_location_event": "stdout",
"whitelisted_oauth_urls": [],
"oauth_client_secret": "",
+ "oauth_user_attribute": "unique_name",
Review Comment:
@zrhoffman do we need it there?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a diff in pull request #7619: Oauth go dependency change
Posted by "srijeet0406 (via GitHub)" <gi...@apache.org>.
srijeet0406 commented on code in PR #7619:
URL: https://github.com/apache/trafficcontrol/pull/7619#discussion_r1256450056
##########
dev/traffic_ops/cdn.json:
##########
@@ -10,6 +10,7 @@
"log_location_event": "stdout",
"whitelisted_oauth_urls": [],
"oauth_client_secret": "",
+ "oauth_user_attribute": "unique_name",
Review Comment:
Would these also need to be added to the ansible main.yml file?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org