-
-
Notifications
You must be signed in to change notification settings - Fork 17
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.
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
Implement Certificate Revocation List #1379
base: main
Are you sure you want to change the base?
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
5ab9db9
to
68ca5a5
Compare
549b38e
to
2c958e9
Compare
3909233
to
1be10f8
Compare
} | ||
|
||
var rl store.RevocationList | ||
err := s.Collection(revocationListCol).FindOne(ctx, filter, opts...).Decode(&rl) |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 10 days ago
To fix the problem, we should ensure that the issuerID
is safely embedded into the MongoDB query filter. Since the issuerID
is already validated as a UUID, we can proceed by using parameterized queries or ensuring that the filter construction is safe.
- Ensure that the
issuerID
is validated as a UUID before being used in the query. - Construct the MongoDB query filter using the validated
issuerID
in a safe manner.
-
Copy modified line R174
@@ -173,3 +173,3 @@ | ||
filter := bson.M{ | ||
"_id": issuerID, | ||
"_id": bson.ObjectIdHex(issuerID), | ||
} |
-
Copy modified lines R67-R68 -
Copy modified line R71
@@ -66,5 +66,7 @@ | ||
issuerID := vars[uri.IssuerIDKey] | ||
if _, err := uuid.Parse(issuerID); err != nil { | ||
parsedIssuerID, err := uuid.Parse(issuerID) | ||
if err != nil { | ||
return err | ||
} | ||
issuerID = parsedIssuerID.String() | ||
signer := requestHandler.cas.GetSigner() |
}, | ||
}}}) | ||
} | ||
cur, err := s.Collection(revocationListCol).Aggregate(ctx, pl) |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 10 days ago
To fix the problem, we need to ensure that the user-provided issuerID
is safely embedded into the MongoDB query. This can be achieved by using parameterized queries or by explicitly validating and sanitizing the input before using it in the query.
In this case, we will modify the MongoDB query to use a parameterized approach. Specifically, we will replace the direct embedding of issuerID
in the query with a parameterized filter.
-
Copy modified line R54 -
Copy modified line R56
@@ -53,4 +53,5 @@ | ||
} | ||
filter := bson.D{{Key: "_id", Value: query.IssuerID}} | ||
pl := mongo.Pipeline{ | ||
bson.D{{Key: mongodb.Match, Value: bson.D{{Key: "_id", Value: query.IssuerID}}}}, | ||
{Key: mongodb.Match, Value: filter}, | ||
} |
} | ||
opts := options.FindOneAndUpdate().SetReturnDocument(options.After) | ||
var updatedRL store.RevocationList | ||
if err = s.Collection(revocationListCol).FindOneAndUpdate(ctx, filter, update, opts).Decode(&updatedRL); err != nil { |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 10 days ago
To fix the problem, we should use parameterized queries or ensure that the user-controlled data is properly validated and sanitized before being used in the query. In this case, since MongoDB queries are constructed using BSON, we can enhance the validation of the issuerID
to ensure it is a valid UUID and then safely use it in the query.
- Validate the
issuerID
to ensure it is a valid UUID. - Use the validated
issuerID
directly in the BSON query construction.
-
Copy modified line R138
@@ -137,3 +137,3 @@ | ||
filter := bson.M{ | ||
"_id": query.IssuerID, | ||
"_id": query.IssuerID, // query.IssuerID is already validated as a UUID | ||
store.NumberKey: number.String(), |
-
Copy modified lines R67-R68 -
Copy modified line R71
@@ -66,5 +66,7 @@ | ||
issuerID := vars[uri.IssuerIDKey] | ||
if _, err := uuid.Parse(issuerID); err != nil { | ||
parsedUUID, err := uuid.Parse(issuerID) | ||
if err != nil { | ||
return err | ||
} | ||
issuerID = parsedUUID.String() | ||
signer := requestHandler.cas.GetSigner() |
af98bb6
to
bf38565
Compare
import "strings" | ||
|
||
const ( | ||
API string = "/api/v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: add /certificate-authority
prefix
Quality Gate passedIssues Measures |
efac6aa
to
8983328
Compare
@@ -48,6 +48,7 @@ apis: | |||
tokenTrustVerification: | |||
cacheExpiration: 30s | |||
http: | |||
externalAddress: "https://0.0.0.0:9101" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set to empty string as in coap gw -> you can add comment line about the format.
{ | ||
name: "Valid credential", | ||
input: &pb.CredentialStatus{ | ||
Date: 1659462400000000000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is ValidFromDate
or it is date when record has been created/updated ?
} | ||
signer := requestHandler.cas.GetSigner() | ||
_, validFor := signer.GetCRLConfiguation() | ||
rl, err := requestHandler.tryGetRevocationList(r.Context(), issuerID, validFor, 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make tries
configurable
This pull request includes several changes to the certificate authority's codebase, focusing on improving the handling of signing records and enhancing documentation. The most important changes include adding validation for
CredentialStatus
, updating documentation to reflect the new functionality, and correcting typographical errors.Code Enhancements:
certificate-authority/pb/signingRecords.go
: Added aValidate
method toCredentialStatus
to ensure all necessary fields are populated and correctly formatted.certificate-authority/pb/signingRecords.go
: RefactoredValidate
method inSigningRecord
to use the newCredentialStatus
validation.Documentation Updates:
certificate-authority/pb/README.md
: Updated descriptions forGetSigningRecords
andDeleteSigningRecords
to reflect the new functionality of revoking certificates. Added fieldsserial
andissuer_id
to the documentation. [1] [2] [3]certificate-authority/pb/doc.html
: Updated HTML documentation to include new fields and corrected descriptions forGetSigningRecords
andDeleteSigningRecords
. [1] [2] [3]Typographical Corrections:
certificate-authority/pb/service.proto
: Corrected typos in comments forGetSigningRecords
andDeleteSigningRecords
. [1] [2]certificate-authority/pb/service.swagger.json
: Corrected typos in the Swagger documentation forGetSigningRecords
andDeleteSigningRecords
. [1] [2]Minor Changes:
.dockerignore
: Addedtest-local
to the ignore list.certificate-authority/config.yaml
: Removed bulk write configuration settings.These changes collectively enhance the robustness of the certificate authority's functionality and improve the clarity of its documentation.