ocket8888 commented on code in PR #7555: URL: https://github.com/apache/trafficcontrol/pull/7555#discussion_r1269995295
########## experimental/traffic-portal/src/app/core/certs/certs.module.ts: ########## @@ -0,0 +1,48 @@ +/* +* Licensed under the Apache License, Version 2.0 (the "License"); +* you may not use this file except in compliance with the License. +* You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ +import { CommonModule } from "@angular/common"; +import { NgModule } from "@angular/core"; +import { RouterModule, Routes } from "@angular/router"; + +import { AppUIModule } from "src/app/app.ui.module"; +import { CertViewerComponent } from "src/app/core/certs/cert-viewer/cert-viewer.component"; +import { SharedModule } from "src/app/shared/shared.module"; + +import { CertAuthorComponent } from "./cert-author/cert-author.component"; +import { CertDetailComponent } from "./cert-detail/cert-detail.component"; + +export const ROUTES: Routes = [ + {component: CertViewerComponent, path: "ssl/ds/:xmlId"}, + {component: CertViewerComponent, path: "ssl"} +]; + +/** + * + */ +@NgModule({ Review Comment: blank JSDoc; I'd just explain why this functionality is sequestered. ########## experimental/traffic-portal/src/app/core/certs/certs.module.ts: ########## @@ -0,0 +1,48 @@ +/* +* Licensed under the Apache License, Version 2.0 (the "License"); +* you may not use this file except in compliance with the License. +* You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ +import { CommonModule } from "@angular/common"; +import { NgModule } from "@angular/core"; +import { RouterModule, Routes } from "@angular/router"; + +import { AppUIModule } from "src/app/app.ui.module"; +import { CertViewerComponent } from "src/app/core/certs/cert-viewer/cert-viewer.component"; +import { SharedModule } from "src/app/shared/shared.module"; + +import { CertAuthorComponent } from "./cert-author/cert-author.component"; +import { CertDetailComponent } from "./cert-detail/cert-detail.component"; + +export const ROUTES: Routes = [ + {component: CertViewerComponent, path: "ssl/ds/:xmlId"}, + {component: CertViewerComponent, path: "ssl"} Review Comment: RE: these routes do we anticipate using this module for non-SSL certificates? And do you think it would be confusing to eliminate the `ds/` portion - because the XMLID is required and makes the route unique without requiring that part. ########## experimental/traffic-portal/src/app/core/certs/cert-viewer/cert-viewer.component.scss: ########## @@ -0,0 +1,33 @@ +/* +* Licensed under the Apache License, Version 2.0 (the "License"); +* you may not use this file except in compliance with the License. +* You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ + +.tab-content { + margin-top: 5px; + display: grid; + + &:is(form), .mat-accordion { + width: 98%; + margin: auto auto 15px; + } +} + +.private-text { Review Comment: I looked into the styling on this, and the issue is that the Angular Material styling is more specific because it excludes `disabled` controls. So all you have to do is change this selector to match that specificity - however, we don't want it to not handle disabled textareas, since those can still be private. So just split the styling up to be more explicit: ```scss .private-text { &:not(:focus) { color: transparent; text-shadow: 0 0 8px rgba(0,0,0,0.5); } &:focus { color: inherit; text-shadow: none; } } ``` that's like one extra line, but doesn't require `!important`. If you wanna argue that that's somehow worse than just using `!important`, I'll hear you out, but just note that it can be done. ########## experimental/traffic-portal/src/app/shared/navigation/navigation.service.ts: ########## @@ -186,6 +186,10 @@ export class NavigationService { { href: "/core/iso-gen", name: "Generate System ISO" + }, + { + href: "/core/certs/ssl", + name: "Inspect Cert" Review Comment: I think this should spell out "Certificate" - I'd even vote to be more specific: "SSL Certificate" ########## experimental/traffic-portal/src/app/api/delivery-service.service.ts: ########## @@ -441,4 +442,16 @@ export class DeliveryServiceService extends APIService { this.deliveryServiceTypes = r; return r; } + + /** + * Gets a Delivery Service's SSL Keys + * + * @param ds The delivery service xmlid or object + * @returns The DS ssl keys + */ + public async getSSLKeys(ds: string | ResponseDeliveryService): Promise<ResponseDeliveryServiceSSLKey> { + const xmlId = typeof ds === "string" ? ds : ds.xmlId; Review Comment: Coverage of branches in this file is reduced by this PR from 100% to 97.36% - that's because the path where a string is passed to this method is not covered. ########## experimental/traffic-portal/src/app/core/certs/cert-viewer/cert-viewer.component.ts: ########## @@ -0,0 +1,187 @@ +/* +* Licensed under the Apache License, Version 2.0 (the "License"); +* you may not use this file except in compliance with the License. +* You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ +import { Component, OnInit, ViewChild } from "@angular/core"; +import { FormControl } from "@angular/forms"; +import { MatTabGroup } from "@angular/material/tabs"; +import { ActivatedRoute } from "@angular/router"; +import { pki } from "node-forge"; +import { type ResponseDeliveryServiceSSLKey } from "trafficops-types"; + +import { DeliveryServiceService } from "src/app/api"; + +/** + * What type of cert is it + */ +type CertType = "Root" | "Client" | "Intermediate" | "Unknown" | "Error"; + +/** + * Detected order of the cert chain + */ +type CertOrder = "Client -> Root" | "Root -> Client" | "Unknown" | "Single"; + +/** + * Wrapper around Certificate that contains additional fields + */ +export interface AugmentedCertificate extends pki.Certificate { + type: CertType; + parseError: boolean; +} + +export const NULL_CERT = pki.createCertificate() as AugmentedCertificate; +NULL_CERT.type = "Error"; +NULL_CERT.parseError = true; + +/** + * Controller for the Cert Viewer component. + */ +@Component({ + selector: "tp-cert-viewer", + styleUrls: ["./cert-viewer.component.scss"], + templateUrl: "./cert-viewer.component.html" +}) +export class CertViewerComponent implements OnInit { + public cert!: ResponseDeliveryServiceSSLKey; + public inputCert = ""; + public dsCert = false; + + public certChain: Array<AugmentedCertificate> = []; + public certOrder: CertOrder | undefined; + public privateKeyFormControl = new FormControl(""); + + @ViewChild("matTab") public matTab!: MatTabGroup; + constructor( + private readonly route: ActivatedRoute, + private readonly dsAPI: DeliveryServiceService) { + } + + /** + * newCert creates a cert from an input string. + * + * @param input The text to read as a cert + * @private + * @returns Resultant Cert + */ + private newCert(input: string): AugmentedCertificate { + try { + return pki.certificateFromPem(input) as AugmentedCertificate; + } catch (e) { + console.error(`ran into issue creating certificate from input ${input}`, e); + return NULL_CERT; + } + } + + /** + * process takes the Cert Chain text input and parses it. + * + * @param uploaded if the certificate was uploaded by the client. + */ + public process(uploaded: boolean = false): void { + this.inputCert = this.inputCert.replace(/\r\n/g, "\n"); + const parts = this.inputCert.split("-\n-"); + const certs = new Array<AugmentedCertificate>(parts.length); + for(let i = 1; i < parts.length; ++i) { + parts[i-1] += "-"; + parts[i] = `-${parts[i]}`; + certs[i-1] = this.newCert(parts[i - 1]); + } + certs[certs.length-1] = this.newCert(parts[parts.length - 1]); + const assignType = (c: AugmentedCertificate, i: number): void => { + if(c.parseError) { + return; + } + if (i === 0) { + c.type = "Root"; + } else if (i === certs.length - 1) { + c.type = "Client"; + } else { + c.type = "Intermediate"; + } + }; + const chain = this.reOrderRootFirst(certs); + chain.forEach(assignType); + this.certChain = chain; + + if(this.matTab && uploaded) { + this.matTab.selectedIndex = 1; + } + } + + /** + * reOrderRootFirst sorts a cert chain with the root being first if possible. + * + * @param certs The list of certs to reorder + * @returns The processed certs + */ + public reOrderRootFirst(certs: Array<AugmentedCertificate>): Array<AugmentedCertificate> { + let rootFirst = false; + let invalid = false; + for(let i = 1; i < certs.length; ++i){ + const first = certs[i-1]; + const next = certs[i]; + if(first.parseError) { + invalid = true; + continue; + } else if (next.parseError) { + invalid = true; + continue; + } + if (first.issued(next)) { + rootFirst = true; + } else if (next.issued(first)) { + rootFirst = false; + } else { + invalid = true; + console.error(`Cert chain is invalid, cert ${i-1} and ${i} are not related`); + } + } + + if (certs.length === 1) { + if (certs[0].parseError) { + invalid = true; + } else { + this.certOrder = "Single"; + return certs; + } + } + if (invalid) { + this.certOrder = "Unknown"; + return certs; + } + + if(rootFirst) { + this.certOrder = "Root -> Client"; + return certs; + } + this.certOrder = "Client -> Root"; + certs = certs.reverse(); + return certs; + } + + /** + * Checks if we are a DS cert or any user provided cert. + */ + public async ngOnInit(): Promise<void> { + const ID = this.route.snapshot.paramMap.get("xmlId"); + if (ID === null) { + this.dsCert = false; + return; + } + this.cert = await this.dsAPI.getSSLKeys(ID); Review Comment: if I navigate to the SSL certificate inspection page for a non-existent DS (e.g. if I click on an old link for a DS that has since been deleted), then `dsCert` will be `true`, but the actual page shown is the one for manual input and inspection - _not_ inspecting a DS certificate. Also, the URL will remain showing me the nonexistent XMLID - it should instead replace the routing state to the non-DS certificate viewer URL. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@trafficcontrol.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
