-
Notifications
You must be signed in to change notification settings - Fork 17
New cert #174
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
base: master
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR implements a new certificate issuance system for the training plugin, replacing the previous certificate code-based completion system with direct PDF generation and download functionality.
- Adds new API endpoints for certificate issuance, download, and reset operations
- Replaces the certificate code display with a modal-based certificate generation UI
- Implements a complete certificate service for PDF generation with MITRE Caldera branding
Reviewed Changes
Copilot reviewed 4 out of 8 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| hook.py | Adds new API route registrations for certificate operations |
| gui/views/training.vue | Replaces certificate code UI with PDF generation modal and download functionality |
| app/training_api.py | Implements certificate API endpoints with completion validation and PDF generation |
| app/c_certificate_service.py | New service class for PDF certificate generation using ReportLab |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
app/c_certificate_service.py
Outdated
| if os.path.exists(FONT_REGULAR): | ||
| pdfmetrics.registerFont(ttfonts.TTFont('CertRegular', FONT_REGULAR)) | ||
| if os.path.exists(FONT_BOLD): | ||
| pdfmetrics.registerFont(ttfonts.TTFont('CertBold', FONT_BOLD)) |
Copilot
AI
Sep 29, 2025
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.
The variables FONT_REGULAR and FONT_BOLD are referenced but never defined in this file, which will cause NameError exceptions when _register_fonts_if_available is called.
app/training_api.py
Outdated
| if not cert_name or not display_name: | ||
| raise web.HTTPBadRequest(text='certificate and name required') | ||
|
|
Copilot
AI
Sep 29, 2025
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 validation check is duplicated from lines 144-145. The duplicate check should be removed to avoid redundant validation logic.
| if not cert_name or not display_name: | |
| raise web.HTTPBadRequest(text='certificate and name required') | |
| # (Duplicate validation removed) |
| raise web.HTTPNotFound(text='Certificate does not exist') | ||
| cert_id = getattr(cert, 'unique', None) or getattr(cert, 'identifier', None) or getattr(cert, 'id', None) or cert.name |
Copilot
AI
Sep 29, 2025
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.
The cert_id assignment on line 199 is duplicated from line 196. This logic should be extracted to avoid code duplication.
gui/views/training.vue
Outdated
| (flag) => flag.badge_name === badgeList.value[badgeIndex - 1].name | ||
| ); | ||
| if (!earlierFlags[earlierFlags.length - 1].completed) { | ||
| if (!earlierFlags.length || !earlierFlags[earlierFlags.length - 1].completed) { |
Copilot
AI
Sep 29, 2025
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.
The added check for !earlierFlags.length could cause issues if earlierFlags is an empty array but the logic expects to access the last element. Consider using optional chaining or a more explicit check.
| if (!earlierFlags.length || !earlierFlags[earlierFlags.length - 1].completed) { | |
| if (!earlierFlags.length || !earlierFlags[earlierFlags.length - 1]?.completed) { |
Updated font settings to use Helvetica fonts directly.
Removed validation for certificate and display name.
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.
Pull Request Overview
Copilot reviewed 4 out of 8 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| LOGO_TOP_CENTER = 'plugins/training/static/templates/mitreCaldera.png' | ||
| LOGO_BOTTOM_LEFT = 'plugins/training/static/templates/caldera_logo.png' |
Copilot
AI
Oct 6, 2025
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.
Corrected spacing in variable names for consistency.
| LOGO_TOP_CENTER = 'plugins/training/static/templates/mitreCaldera.png' | |
| LOGO_BOTTOM_LEFT = 'plugins/training/static/templates/caldera_logo.png' | |
| LOGO_TOP_CENTER = 'plugins/training/static/templates/mitreCaldera.png' | |
| LOGO_BOTTOM_LEFT = 'plugins/training/static/templates/caldera_logo.png' |
| out_pdf = os.path.join(OUT_DIR, base) | ||
|
|
||
| # Visible strings | ||
| display_cert_title = "MITRE Caldera™ User" |
Copilot
AI
Oct 6, 2025
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.
Hard-coded certificate title should be configurable or derived from the cert_name parameter to support different certificate types.
| display_cert_title = "MITRE Caldera™ User" | |
| display_cert_title = cert_name |
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.
@deacon-mp thoughts on this?
| const byteChars = atob(data.pdf_bytes); | ||
| const byteNums = new Array(byteChars.length); | ||
| for (let i = 0; i < byteChars.length; i++) byteNums[i] = byteChars.charCodeAt(i); | ||
| const blob = new Blob([new Uint8Array(byteNums)], { type: 'application/pdf' }); |
Copilot
AI
Oct 6, 2025
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.
The manual byte array conversion is inefficient. Consider using Uint8Array.from() for better performance: const byteArray = Uint8Array.from(atob(data.pdf_bytes), c => c.charCodeAt(0));
| const byteChars = atob(data.pdf_bytes); | |
| const byteNums = new Array(byteChars.length); | |
| for (let i = 0; i < byteChars.length; i++) byteNums[i] = byteChars.charCodeAt(i); | |
| const blob = new Blob([new Uint8Array(byteNums)], { type: 'application/pdf' }); | |
| const byteArray = Uint8Array.from(atob(data.pdf_bytes), c => c.charCodeAt(0)); | |
| const blob = new Blob([byteArray], { type: 'application/pdf' }); |
| if not cert_name or not display_name: | ||
| raise web.HTTPBadRequest(text='certificate and name required') | ||
|
|
||
| user_id = request.headers.get('X-User-ID') or request.headers.get('KEY') or 'unknown' |
Copilot
AI
Oct 6, 2025
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.
The user ID extraction logic is duplicated across multiple methods. Consider extracting this to the existing _request_user_id method.
| user_id = request.headers.get('X-User-ID') or request.headers.get('KEY') or 'unknown' | |
| user_id = await self._request_user_id(request) |
| except web.HTTPException: | ||
| raise | ||
| except Exception: | ||
| logging.exception("issue_certificate failed") |
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.
can we log the exception details?
| if not cert_name: | ||
| raise web.HTTPBadRequest(text='certificate required') | ||
| user_id = body.get('user_id') or self._request_user_id(request) | ||
| instance_id = hashlib.sha256(self.cert_service.secret).hexdigest()[:12] |
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.
best to turn this into a helper function since we're calling this logic at least 3 times
| raise web.HTTPBadRequest(text='certificate and name required') | ||
|
|
||
| # auth-ish identity; same as issue_certificate | ||
| user_id = request.headers.get('X-User-ID') or request.headers.get('KEY') or 'unknown' |
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.
replace with helper func
Description
Static pdf build of the training certificate.
export TRAINING_CERT_BYPASS=1
python3 server.py --insecure
DL=$(jq -r '.download' /tmp/issue.json)
curl -s -H "KEY: ADMIN123" -o cert-link.pdf "http://localhost:8888${DL// /%20}"
curl -s -X POST http://localhost:8888/plugin/training/certificate/issue -H "KEY: ADMIN123" -H "Content-Type: application/json" -d '{"certificate":"User Certificate","name":"Test User"}' | tee /tmp/issue.json
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: