-
Notifications
You must be signed in to change notification settings - Fork 344
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
TPv2 Add the ability to inspect a cert/DS #7555
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7555 +/- ##
=============================================
+ Coverage 29.84% 65.27% +35.43%
Complexity 98 98
=============================================
Files 794 318 -476
Lines 84884 12531 -72353
Branches 908 931 +23
=============================================
- Hits 25330 8180 -17150
+ Misses 57424 3992 -53432
+ Partials 2130 359 -1771
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 484 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
200198a
to
64604c3
Compare
experimental/traffic-portal/src/app/api/testing/delivery-service.service.ts
Outdated
Show resolved
Hide resolved
experimental/traffic-portal/src/app/core/certs/cert-viewer/cert-viewer.component.html
Show resolved
Hide resolved
experimental/traffic-portal/src/app/core/certs/cert-viewer/cert-viewer.component.html
Outdated
Show resolved
Hide resolved
experimental/traffic-portal/src/app/core/certs/cert-viewer/cert-viewer.component.html
Outdated
Show resolved
Hide resolved
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.
node-forge
is kinda big - I'm just wondering if the things you need to do can be accomplished with a SubtleCrypto
interface? It's pretty basic, but if it does have the required functionality then it'd be the superior choice IMO.
experimental/traffic-portal/src/app/api/testing/delivery-service.service.ts
Outdated
Show resolved
Hide resolved
experimental/traffic-portal/src/app/core/certs/cert-author/cert-author.component.ts
Outdated
Show resolved
Hide resolved
experimental/traffic-portal/src/app/core/certs/cert-author/cert-author.component.ts
Outdated
Show resolved
Hide resolved
experimental/traffic-portal/src/app/core/certs/cert-detail/cert-detail.component.scss
Outdated
Show resolved
Hide resolved
experimental/traffic-portal/src/app/core/certs/cert-detail/cert-detail.component.ts
Outdated
Show resolved
Hide resolved
experimental/traffic-portal/src/app/core/certs/cert-detail/cert-detail.component.html
Outdated
Show resolved
Hide resolved
experimental/traffic-portal/src/app/core/certs/cert-viewer/cert-viewer.component.scss
Outdated
Show resolved
Hide resolved
experimental/traffic-portal/src/app/core/certs/cert-viewer/cert-viewer.component.ts
Outdated
Show resolved
Hide resolved
I would've loved to use a native browser api but the ones that exist are insufficient and at best would require significant amounts of code to replicate the current behavior. |
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.
Logically the code LGTM. Tested it on local and the design (along with solution) works as expected.
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 DS certtificate inspector looks to not be navigable to me; did you want to add a link on DS cards or details pages for content routing types that allow HTTPS?
experimental/traffic-portal/src/app/core/certs/cert-viewer/cert-viewer.component.scss
Show resolved
Hide resolved
experimental/traffic-portal/src/app/core/certs/cert-viewer/cert-viewer.component.ts
Outdated
Show resolved
Hide resolved
experimental/traffic-portal/src/app/shared/navigation/navigation.service.ts
Outdated
Show resolved
Hide resolved
7d4d2af
to
666f28c
Compare
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.
Looks fine so far, need to test before I approve
This PR adds the ability to inspect a user provided cert chain or the one from a specific DS. This requires the use of a new library
node-forge
. This library is relatively large (~200Kb compressed) and as such the functionality is relegated to it's own module.Which Traffic Control components are affected by this PR?
What is the best way to verify this PR?
Ensure the SSL chunk is only loaded when navigating to one of the two endpoints,
core/certs/ssl
andcore/certs/ssl/ds/:xmlId
. The generic endpoint has a sidebar entry underOther
and should work.For DS cert (which needs to be navigated to manually), there should be two tabs. One displaying the information that TO provides, and the other displaying the processed cert info.
The generic cert also has two tabs, the processed tab like with DS cert, and an input tab where the user can input the cert. The only validation that should occur on this tab is ensuring the cert field is not empty.
The processed cert screen should always show the chain in root -> client order. It should also display the detected order (if possible). In case of a cert that doesn't match or a cert doesn't parse it should properly update the order to "Unknown" and that specific certs name to "Error".
PR submission checklist