Skip to content

Commit

Permalink
#15 - when idCard service (https://github.com/citizenos/id-auth) is c…
Browse files Browse the repository at this point in the history 
…onfigured, dont trust X-SSL-Client-Cert header
  • Loading branch information
@tiblu
tiblu committed Aug 29, 2018
1 parent e10f089 commit 7b95c9e
Show file tree
Hide file tree
Showing 5 changed files with 7 additions and 9 deletions.
1 change: 0 additions & 1 deletion app.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ if (app.get('env') === 'production' || app.get('env') === 'test') {
app.set('trust proxy', true); // http://expressjs.com/guide/behind-proxies.html
}


app.use('/documentation', swaggerUi.serve, swaggerUi.setup(swaggerDocument));
var prerender = require('prerender-node');
prerender.set('prerenderServiceUrl', config.services.prerender.serviceUrl).set('prerenderToken', config.services.prerender.apiKey);
Expand Down
1 change: 1 addition & 0 deletions config/custom-environment-variables.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"token": "CITIZENOS_SERVICES_DIGIDOC_TOKEN"
},
"idCard": {
"serviceUrl": "CITIZENOS_SERVICES_IDCARD_SERVICE_URL",
"apiKey": "CITIZENOS_SERVICES_IDCARD_APIKEY"
},
"smartId": {
Expand Down
4 changes: 0 additions & 4 deletions config/default.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,6 @@
"ssl": true,
"rejectUnauthorized": true
},
"idCard": {
"serviceUrl": "https://id.citizenos.com:8443/info",
"apiKey": null
},
"smartId": {
"hostname": "smartid.citizenos.com:8001",
"authorizeToken": "m3EvIdswsiIpeOXqXb185fgoS9h0zpu5jrweZWLVGeVR5FoQpE",
Expand Down
4 changes: 0 additions & 4 deletions config/test.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,6 @@
"rejectUnauthorized": false,
"apikey": "LUVORFBXdUhrb0RMLXhzekREcXpjR1NsU1BBeTZRWmJGRUR4blZpX3YzYjJRa3FZ"
},
"idCard": {
"serviceUrl": "https://dev.id.citizenos.com:3002/info",
"apiKey": "ZWeCMMQSoIpOnavNnEo2daBjXbmG3PoEWgNnQ13EtZ9jF43gkInxok5fIioX"
},
"smartId": {
"hostname": "sid.demo.sk.ee",
"apiPath": "/smart-id-rp/v1",
Expand Down
6 changes: 6 additions & 0 deletions routes/api/auth.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,12 @@ module.exports = function (app) {
var token = req.body.token; // Token to access the ID info service
var cert = req.headers['x-ssl-client-cert'];

if (config.services.idCard && cert) {
logger.error('X-SSL-Client-Cert header is not allowed when ID-card service is enabled. IF you trust your proxy, sending the X-SSL-Client-Cert, delete the services.idCard from your configuration.');

return res.badRequest('X-SSL-Client-Cert header is not allowed when ID-card proxy service is enabled.');
}

if (!token && !cert) {
logger.warn('Missing required parameter "token" OR certificate in X-SSL-Client-Cert header. One must be provided!', req.path, req.headers);

Expand Down

4 comments on commit 7b95c9e

@moll
Copy link

@moll moll commented on 7b95c9e Aug 30, 2018 •
edited

Choose a reason for hiding this comment

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

I strongly recommend you get cert from the header only if the proxy setting is unset. That'd be more whitelist-y than throwing an error, a blacklist approach. The latter are always waiting to be accidentally sidestepped.

@tiblu
Copy link
Member Author

@tiblu tiblu commented on 7b95c9e Aug 30, 2018 •
edited

Choose a reason for hiding this comment

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

@moll Rephrasing just in case - use the cert when Express's trust proxy is set to true? Seems logical.

Which reminds me right now it's environment based, but I guess we should let it be configurable?
https://github.com/citizenos/citizenos-api/blob/master/app.js#L43

@moll
Copy link

@moll moll commented on 7b95c9e Aug 30, 2018

Choose a reason for hiding this comment

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

Nah, I meant the idCard proxy setting. Express's proxy setting is true probably everywhere, as even Heroku has a proxy in front of your app.

@tiblu
Copy link
Member Author

@tiblu tiblu commented on 7b95c9e Aug 30, 2018

Choose a reason for hiding this comment

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

OK, I agree. The main point is that the "if" is easy to disappear and then the code becomes vulnerable.
It makes sense to make 2 totally different code paths depending if cert is to be used or not.

Please sign in to comment.