Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve handling of challenging OAuth clients #1684

Merged
merged 3 commits into from
Apr 10, 2015
Merged

Improve handling of challenging OAuth clients #1684

merged 3 commits into from
Apr 10, 2015

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Apr 10, 2015

  • Protect browsers from basic-auth CSRF attacks by requiring a custom header ("X-CSRF-Token")
  • Make osc send the custom header
  • Provide a more helpful warning when authenticating via WWW-Authenticate challenge is not supported by the current OAuth config
  • Surface warnings in osc when login fails (because of bad credentials or because cli login isn't supported)
  • Tweak the browser request token page to show using the token to log in

@liggitt
Copy link
Contributor Author

liggitt commented Apr 10, 2015

@jcantrill you'll need to add an "X-CSRF-Token" header (any value is allowed, it just has to be set) when making requests to /oauth/authorize with the openshift-challenging-client client_id.

@liggitt
Copy link
Contributor Author

liggitt commented Apr 10, 2015

@brenton I think you also made use of the openshift-challenging-client... a custom header is now required when using that client to prevent CSRF browser attacks

@liggitt
Copy link
Contributor Author

liggitt commented Apr 10, 2015

@deads2k review

@liggitt liggitt changed the title Protect browsers from basic-auth CSRF attacks Improve handling of challenging OAuth clients Apr 10, 2015
@liggitt
Copy link
Contributor Author

liggitt commented Apr 10, 2015

@TomasTomecek not sure what mechanism you are using to get OAuth tokens, but if you are using something that requires a basic-auth "WWW-Authenticate: Basic" challenge from the OpenShift OAuth server, you may need to start sending a "X-CSRF-Token: 1" header to continue getting the challenges.

@TomasTomecek
Copy link
Contributor

@liggitt am using basic auth for testing since I don't have kerberos on my dev instance. On prod it's requestheader provider. Thanks for headsup.

deads2k
deads2k reviewed Apr 10, 2015
View reviewed changes
pkg/auth/oauth/handlers/default_auth_handler.go

var (
// http://tools.ietf.org/html/rfc2616#section-14.46
warningRegex = regexp.MustCompile(`([0-9]{3}) ([^ ]+) "([^"]+)".*`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just going to trust you on this...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh great, it's used. Three numbers, a space, one or more non-space characters, a space, a non-empty quoted string, and then whatever I want? If you're going to do this, handle the date instead of punting it and have fixed start and end.

Surely this is built into some library.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, my pre-8am mind wants you to write tests for this regex, because they're always broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clarified? commented and tested, anyway...

@deads2k
Copy link
Contributor

deads2k commented Apr 10, 2015

Making me read a regex first thing in the morning makes me grumpy.

@jcantrill
Copy link
Contributor

@liggitt Thanks. Updated the java client: openshift/openshift-restclient-java#6

@liggitt
Copy link
Contributor Author

liggitt commented Apr 10, 2015

comments addressed

@deads2k
Copy link
Contributor

deads2k commented Apr 10, 2015

lgtm. Well, I should clarify. That regex makes me want to run and hide, but now that it looks like a standard regex that we definitely can't read, squash and merge at will.

@liggitt
Copy link
Contributor Author

liggitt commented Apr 10, 2015

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1544/) (Image: devenv-fedora_1257)

@openshift-bot
Copy link
Contributor

Evaluated for origin up to 7ecd59d

openshift-bot pushed a commit that referenced this pull request Apr 10, 2015
Merge pull request #1684 from liggitt/csrf_basic_auth
075b4bc 
Merged by openshift-bot
@openshift-bot openshift-bot merged commit 075b4bc into openshift:master Apr 10, 2015
@liggitt liggitt deleted the csrf_basic_auth branch April 13, 2015 15:26
@smarterclayton smarterclayton modified the milestone: 0.5.0 (beta3) Apr 23, 2015
jpeeler pushed a commit to jpeeler/origin that referenced this pull request Mar 1, 2018
Squashed 'cmd/service-catalog/go/src/github.com/kubernetes-incubator/…
f72ed83 
…service-catalog/' changes from b758460ba7..c3e3071633

c3e3071633 origin build: add origin tooling
97ddbab chart changes for v0.1.9 (openshift#1776)
b5168a7 Add unit tests for class, instance, plan backends in svcat (openshift#1763)
97d11cb prometheus: only return catalog specific metrics (openshift#1774)
0fb00e3 Bump dependency on go-open-service-broker-client to 0.0.4 (openshift#1775)
0a9f1e4 Reset RemovedFromBrokerCatalog when broker re-adds a removed service class (openshift#1770)
28ec5ed Bump dependency on go-open-service-broker-client to 0.0.3 (openshift#1768)
ca83d18 handle binding deletion that occurs during async bind (openshift#1760)
858d467 2 of 4 fixes for golang 1.10 (openshift#1764)
656156b Add unit tests for binding and broker backends in svcat
ec05486 In svcat verify service instance exists on unbind (openshift#1750)
e6315a4 fix indentation from openshift#1725 (openshift#1759)
62284da Publish svcat binaries during build (openshift#1725)
8f986ae also build with golang tip and allow tip to fail (openshift#1734)
127561e use pvc for etcd volume (openshift#1684)
7d155e5 Ensure only href-checker runs on docs only commit (openshift#1693)
4ea44c4 log the version and build date on server startup (openshift#1746)
0db9519 allow getting and describing plans with class/plan name combo in svcat (openshift#1743)
b1da783 print schemas when describing plan in svcat (openshift#1740)
7a7fcce Add constraint for go-open-service-broker-client (openshift#1738)
3070003 Increase timeout for broker condition polling in e2e (openshift#1745)
b6878f7 Avoid Setting Authentication header twice (openshift#1685)
5317111 wrap "rm -rf" with docker (openshift#1735)
d7c0bf2 Allow upper case letters in Plan names (openshift#1668)
6b27ba6  Add a constraint on go-autorest  (openshift#1732)
b3de6ec Added validation for ServiceBinding spec ParametersFrom
REVERT: b758460ba7 origin build: modify hard coded path
REVERT: 871582f73a origin build: add origin tooling

git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog
git-subtree-split: c3e3071633b91541cf9f1000d2d5115cdd31de1b
jpeeler pushed a commit to jpeeler/origin that referenced this pull request Mar 1, 2018
Squashed 'cmd/service-catalog/go/src/github.com/kubernetes-incubator/…
f574d6f 
…service-catalog/' changes from b758460ba7..c3e3071633

c3e3071633 origin build: add origin tooling
97ddbab chart changes for v0.1.9 (openshift#1776)
b5168a7 Add unit tests for class, instance, plan backends in svcat (openshift#1763)
97d11cb prometheus: only return catalog specific metrics (openshift#1774)
0fb00e3 Bump dependency on go-open-service-broker-client to 0.0.4 (openshift#1775)
0a9f1e4 Reset RemovedFromBrokerCatalog when broker re-adds a removed service class (openshift#1770)
28ec5ed Bump dependency on go-open-service-broker-client to 0.0.3 (openshift#1768)
ca83d18 handle binding deletion that occurs during async bind (openshift#1760)
858d467 2 of 4 fixes for golang 1.10 (openshift#1764)
656156b Add unit tests for binding and broker backends in svcat
ec05486 In svcat verify service instance exists on unbind (openshift#1750)
e6315a4 fix indentation from openshift#1725 (openshift#1759)
62284da Publish svcat binaries during build (openshift#1725)
8f986ae also build with golang tip and allow tip to fail (openshift#1734)
127561e use pvc for etcd volume (openshift#1684)
7d155e5 Ensure only href-checker runs on docs only commit (openshift#1693)
4ea44c4 log the version and build date on server startup (openshift#1746)
0db9519 allow getting and describing plans with class/plan name combo in svcat (openshift#1743)
b1da783 print schemas when describing plan in svcat (openshift#1740)
7a7fcce Add constraint for go-open-service-broker-client (openshift#1738)
3070003 Increase timeout for broker condition polling in e2e (openshift#1745)
b6878f7 Avoid Setting Authentication header twice (openshift#1685)
5317111 wrap "rm -rf" with docker (openshift#1735)
d7c0bf2 Allow upper case letters in Plan names (openshift#1668)
6b27ba6  Add a constraint on go-autorest  (openshift#1732)
b3de6ec Added validation for ServiceBinding spec ParametersFrom
REVERT: b758460ba7 origin build: modify hard coded path
REVERT: 871582f73a origin build: add origin tooling

git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog
git-subtree-split: c3e3071633b91541cf9f1000d2d5115cdd31de1b
jpeeler pushed a commit to jpeeler/origin that referenced this pull request Mar 8, 2018
Squashed 'cmd/service-catalog/go/src/github.com/kubernetes-incubator/…
fc02f43 
…service-catalog/' changes from b758460ba7..c3e3071633

c3e3071633 origin build: add origin tooling
97ddbab chart changes for v0.1.9 (openshift#1776)
b5168a7 Add unit tests for class, instance, plan backends in svcat (openshift#1763)
97d11cb prometheus: only return catalog specific metrics (openshift#1774)
0fb00e3 Bump dependency on go-open-service-broker-client to 0.0.4 (openshift#1775)
0a9f1e4 Reset RemovedFromBrokerCatalog when broker re-adds a removed service class (openshift#1770)
28ec5ed Bump dependency on go-open-service-broker-client to 0.0.3 (openshift#1768)
ca83d18 handle binding deletion that occurs during async bind (openshift#1760)
858d467 2 of 4 fixes for golang 1.10 (openshift#1764)
656156b Add unit tests for binding and broker backends in svcat
ec05486 In svcat verify service instance exists on unbind (openshift#1750)
e6315a4 fix indentation from openshift#1725 (openshift#1759)
62284da Publish svcat binaries during build (openshift#1725)
8f986ae also build with golang tip and allow tip to fail (openshift#1734)
127561e use pvc for etcd volume (openshift#1684)
7d155e5 Ensure only href-checker runs on docs only commit (openshift#1693)
4ea44c4 log the version and build date on server startup (openshift#1746)
0db9519 allow getting and describing plans with class/plan name combo in svcat (openshift#1743)
b1da783 print schemas when describing plan in svcat (openshift#1740)
7a7fcce Add constraint for go-open-service-broker-client (openshift#1738)
3070003 Increase timeout for broker condition polling in e2e (openshift#1745)
b6878f7 Avoid Setting Authentication header twice (openshift#1685)
5317111 wrap "rm -rf" with docker (openshift#1735)
d7c0bf2 Allow upper case letters in Plan names (openshift#1668)
6b27ba6  Add a constraint on go-autorest  (openshift#1732)
b3de6ec Added validation for ServiceBinding spec ParametersFrom
REVERT: b758460ba7 origin build: modify hard coded path
REVERT: 871582f73a origin build: add origin tooling

git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog
git-subtree-split: c3e3071633b91541cf9f1000d2d5115cdd31de1b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.5.0 (beta3)
Development

Successfully merging this pull request may close these issues.
6 participants
@liggitt @TomasTomecek @deads2k @jcantrill @openshift-bot @smarterclayton