Skip to content

Commit

Permalink
Remove insecure credentialVerificationCallbackAuthEnabled and fix err…
Browse files Browse the repository at this point in the history
…ant test
  • Loading branch information
ottonomy authored and kezike committed Jul 17, 2024
1 parent 203b38f commit dec41e5
Show file tree
Hide file tree
Showing 5 changed files with 122 additions and 30 deletions.
12 changes: 8 additions & 4 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,17 @@

## Fixed
- Avoid reCaptcha timeout by waiting to invoke it until the user submits.
- Enable user to reset exchange from failed back to pending to try submitting again.
- Fix Entra security bug, by using a different secret access token
for an exchange's verification callback endpoint.
- Enable user to reset exchange from failed back to pending to try submitting
again.
- Fix Entra security bug, by using a different secret access token for an
exchange's verification callback endpoint.

## Changed
- Changed cookie timeout from 15m to 1m
- Exchange is updated to invalid state upon invalid JWT presentation. (resettable)
- Exchange is updated to invalid state upon invalid JWT presentation.
(resettable)
- Removed development-only optional `credentialVerificationCallbackAuthEnabled`
Entra workflow configuration option.

## 8.1.1 - 2024-07-08

Expand Down
1 change: 0 additions & 1 deletion configs/combined.example.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,6 @@ app:
apiTenantId: "TODO" # required
verifierDid: "TODO" # required
verifierName: "TODO" # required
credentialVerificationCallbackAuthEnabled: true # optional (default populated in middleware unless overridden here)
acceptedCredentialIssuers: [] # optional (default populated in middleware unless overridden here)
credentialVerificationPurpose: "So that we can evaluate your permission to access the requested resource" # optional (default populated in middleware unless overridden here)
allowRevokedCredentials: false # optional (default populated in middleware unless overridden here)
Expand Down
5 changes: 0 additions & 5 deletions configs/combined.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -527,11 +527,6 @@
"type": "string",
"description": "The ID of the initial step in the workflow"
},
"credentialVerificationCallbackAuthEnabled": {
"type": "boolean",
"description": "Whether to require authentication for the credential verification callback.",
"default": true
},
"acceptedCredentialIssuers": {
"type": "array",
"items": { "type": "string" },
Expand Down
26 changes: 10 additions & 16 deletions lib/workflows/entra-verified-id-workflow.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,12 @@ export class EntraVerifiedIdWorkflowService extends BaseWorkflowService {
} = workflow;
const {acceptedCredentialType} = steps[initialStep];
const defaults = {
credentialVerificationCallbackAuthEnabled: true,
acceptedCredentialIssuers: undefined,
credentialVerificationPurpose: 'To check permission to access resources',
allowRevokedCredentials: false,
validateLinkedDomain: false
};
const {
credentialVerificationCallbackAuthEnabled,
acceptedCredentialIssuers,
credentialVerificationPurpose,
allowRevokedCredentials,
Expand All @@ -79,11 +77,9 @@ export class EntraVerifiedIdWorkflowService extends BaseWorkflowService {
callback: {
url: `${baseUri}/verification/callback`,
state,
...(credentialVerificationCallbackAuthEnabled && {
headers: {
Authorization: apiAccessToken
}
})
headers: {
Authorization: `Bearer ${apiAccessToken}`
}
},
requestedCredentials: [
{
Expand Down Expand Up @@ -188,15 +184,13 @@ export class EntraVerifiedIdWorkflowService extends BaseWorkflowService {
r => r.workflow?.id === exchange.workflowId
);

if(rp.workflow.credentialVerificationCallbackAuthEnabled) {
if(!accessToken) {
res.status(401).send({message: 'Authorization header is required.'});
return;
}
if(exchange.apiAccessToken !== accessToken) {
res.status(401).send({message: 'Invalid access token'});
return;
}
if(!accessToken) {
res.status(401).send({message: 'Authorization header is required.'});
return;
}
if(req.headers.authorization != `Bearer ${exchange.apiAccessToken}`) {
res.status(401).send({message: 'Invalid access token'});
return;
}
let exchangeState; let message;
const results = {};
Expand Down
108 changes: 104 additions & 4 deletions test/mocha/10-api.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,6 @@ describe('OpenCred API - Microsoft Entra Verified ID Workflow',
apiLoginBaseUrl: 'https://login.entra.microsoft.example.com',
verifierDid: 'did:web:example.com',
verifierName: 'Test Entra Verifier',
credentialVerificationCallbackAuthEnabled: false,
initialStep: 'default',
steps: {
default: {
Expand Down Expand Up @@ -643,7 +642,7 @@ describe('OpenCred API - Microsoft Entra Verified ID Workflow',
try {
result = await client.post(`${baseUrl}/verification/callback`, {
headers: {
Authorization: `Bearer ${entraExchange.accessToken}`
Authorization: `Bearer ${entraExchange.apiAccessToken}`
},
json: {
requestId: 'c656dad8-a8fa-4361-baef-51af0c2e428e',
Expand All @@ -667,13 +666,12 @@ describe('OpenCred API - Microsoft Entra Verified ID Workflow',
dateStub.restore();
});

it('should update exchange status after verification with JWT VP token',
it('should encounter auth error using wrong access token on entra callback',
async function() {
const findStub = sinon.stub(database.collections.Exchanges, 'findOne')
.resolves(entraExchange);
const updateStub = sinon.stub(database.collections.Exchanges,
'replaceOne').resolves();
const dateStub = sinon.stub(Date, 'now').returns(1699635246762);
const testVpToken = `
eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtp
ZCI6ImRpZDpleGFtcGxlOjB4YWJjI2tleTEifQ.e
Expand Down Expand Up @@ -743,6 +741,7 @@ describe('OpenCred API - Microsoft Entra Verified ID Workflow',
try {
result = await client
.post(`${baseUrl}/verification/callback`, {
// Use the wrong token (correct: entraExchange.apiAccessToken)
headers: {Authorization: `Bearer ${entraExchange.accessToken}`},
json: {
requestId: 'c656dad8-a8fa-4361-baef-51af0c2e428e',
Expand All @@ -756,6 +755,107 @@ describe('OpenCred API - Microsoft Entra Verified ID Workflow',
err = e;
}

// There should be an error message from the improper authentication
should.equal(err.data?.message, 'Invalid access token');
should.not.exist(result);
findStub.called.should.be.equal(true);
updateStub.called.should.be.equal(false); // It should not have updated
findStub.restore();
updateStub.restore();
});

it('should update exchange status after verification with JWT VP token',
async function() {
const findStub = sinon.stub(database.collections.Exchanges, 'findOne')
.resolves(entraExchange);
const updateStub = sinon.stub(database.collections.Exchanges,
'replaceOne').resolves();
const dateStub = sinon.stub(Date, 'now').returns(1699635246762);
const testVpToken = `
eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtp
ZCI6ImRpZDpleGFtcGxlOjB4YWJjI2tleTEifQ.e
yJpc3MiOiJkaWQ6ZXhhbXBsZTplYmZlYjFmNzEyZ
WJjNmYxYzI3NmUxMmVjMjEiLCJqdGkiOiJ1cm46d
XVpZDozOTc4MzQ0Zi04NTk2LTRjM2EtYTk3OC04Z
mNhYmEzOTAzYzUiLCJhdWQiOiJkaWQ6ZXhhbXBsZ
To0YTU3NTQ2OTczNDM2ZjZmNmM0YTRhNTc1NzMiL
CJuYmYiOjE1NDE0OTM3MjQsImlhdCI6MTU0MTQ5M
zcyNCwiZXhwIjoxNTczMDI5NzIzLCJub25jZSI6I
jM0M3MkRlNGRGEtIiwidnAiOnsiQGNvbnRleHQiO
lsiaHR0cHM6Ly93d3cudzMub3JnLzIwMTgvY3JlZ
GVudGlhbHMvdjEiLCJodHRwczovL3d3dy53My5vc
mcvMjAxOC9jcmVkZW50aWFscy9leGFtcGxlcy92M
SJdLCJ0eXBlIjpbIlZlcmlmaWFibGVQcmVzZW50Y
XRpb24iLCJDcmVkZW50aWFsTWFuYWdlclByZXNlb
nRhdGlvbiJdLCJ2ZXJpZmlhYmxlQ3JlZGVudGlhb
CI6WyJleUpoYkdjaU9pSlNVekkxTmlJc0luUjVjQ
0k2SWtwWFZDSXNJbXRwWkNJNkltUnBaRHBsZUdGd
GNHeGxPbUZpWm1VeE0yWTNNVEl4TWpBME16RmpNa
mMyWlRFeVpXTmhZaU5yWlhsekxURWlmUS5leUp6Z
FdJaU9pSmthV1E2WlhoaGJYQnNaVHBsWW1abFlqR
m1OekV5WldKak5tWXhZekkzTm1VeE1tVmpNakVpT
ENKcWRHa2lPaUpvZEhSd09pOHZaWGhoYlhCc1pTN
WxaSFV2WTNKbFpHVnVkR2xoYkhNdk16Y3pNaUlzS
W1semN5STZJbWgwZEhCek9pOHZaWGhoYlhCc1pTN
WpiMjB2YTJWNWN5OW1iMjh1YW5kcklpd2libUptS
WpveE5UUXhORGt6TnpJMExDSnBZWFFpT2pFMU5ER
TBPVE0zTWpRc0ltVjRjQ0k2TVRVM016QXlPVGN5T
Xl3aWJtOXVZMlVpT2lJMk5qQWhOak0wTlVaVFpYS
WlMQ0oyWXlJNmV5SkFZMjl1ZEdWNGRDSTZXeUpvZ
EhSd2N6b3ZMM2QzZHk1M015NXZjbWN2TWpBeE9DO
WpjbVZrWlc1MGFXRnNjeTkyTVNJc0ltaDBkSEJ6T
2k4dmQzZDNMbmN6TG05eVp5OHlNREU0TDJOeVpXU
mxiblJwWVd4ekwyVjRZVzF3YkdWekwzWXhJbDBzS
W5SNWNHVWlPbHNpVm1WeWFXWnBZV0pzWlVOeVpXU
mxiblJwWVd3aUxDSlZibWwyWlhKemFYUjVSR1ZuY
21WbFEzSmxaR1Z1ZEdsaGJDSmRMQ0pqY21Wa1pXN
TBhV0ZzVTNWaWFtVmpkQ0k2ZXlKa1pXZHlaV1VpT
25zaWRIbHdaU0k2SWtKaFkyaGxiRzl5UkdWbmNtV
mxJaXdpYm1GdFpTSTZJanh6Y0dGdUlHeGhibWM5S
jJaeUxVTkJKejVDWVdOallXeGhkWExEcVdGMElHV
nVJRzExYzJseGRXVnpJRzUxYmNPcGNtbHhkV1Z6U
EM5emNHRnVQaUo5ZlgxOS5LTEpvNUdBeUJORDNMR
FRuOUg3RlFva0VzVUVpOGpLd1hoR3ZvTjNKdFJhN
TF4ck5EZ1hEYjBjcTFVVFlCLXJLNEZ0OVlWbVIxT
klfWk9GOG9HY183d0FwOFBIYkYySGFXb2RRSW9PQ
nh4VC00V05xQXhmdDdFVDZsa0gtNFM2VXgzclNHQ
W1jek1vaEVFZjhlQ2VOLWpDOFdla2RQbDZ6S1pRa
jBZUEIxcng2WDAteGxGQnM3Y2w2V3Q4cmZCUF90W
jlZZ1ZXclFtVVd5cFNpb2MwTVV5aXBobXlFYkxaY
WdUeVBsVXlmbEdsRWRxclpBdjZlU2U2UnR4Snk2T
TEtbEQ3YTVIVHphbllUV0JQQVVIRFpHeUdLWGRKd
y1XX3gwSVdDaEJ6STh0M2twRzI1M2ZnNlYzdFBnS
GVLWEU5NGZ6X1FwWWZnLS03a0xzeUJBZlFHYmciX
X19.ft_Eq4IniBrr7gtzRfrYj8Vy1aPXuFZU-6_a
i0wvaKcsrzI4JkQEKTvbJwdvIeuGuTqy7ipO-EYi
7V4TvonPuTRdpB7ZHOlYlbZ4wA9WJ6mSVSqDACvY
RiFvrOFmie8rgm6GacWatgO4m4NqiFKFko3r58Lu
eFfGw47NK9RcfOkVQeHCq4btaDqksDKeoTrNysF4
YS89INa-prWomrLRAhnwLOo1Etp3E4ESAxg73CR2
kA5AoMbf5KtFueWnMcSbQkMRdWcGC1VssC0tB0Jf
fVjq7ZV6OTyV4kl1-UVgiPLXUTpupFfLRhf9QpqM
BjYgP62KvhIvW8BbkGUelYMetA`;
let result;
let err;
try {
result = await client
.post(`${baseUrl}/verification/callback`, {

headers: {
Authorization: `Bearer ${entraExchange.apiAccessToken}`
},
json: {
requestId: 'c656dad8-a8fa-4361-baef-51af0c2e428e',
requestStatus: 'presentation_verified',
receipt: {
vp_token: testVpToken
}
}
});
} catch(e) {
err = e;
}

should.not.exist(err);
result.status.should.be.equal(200);
findStub.called.should.be.equal(true);
Expand Down

0 comments on commit dec41e5

Please sign in to comment.