Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Feature: Certificate Enhancements #300

Open
wants to merge 1 commit into
base: rc-1.6.0
Choose a base branch
from
Open

Feature: Certificate Enhancements #300

wants to merge 1 commit into from

Conversation

elliot-sabitov
Copy link
Contributor

@elliot-sabitov elliot-sabitov commented Nov 21, 2024 •
edited
Loading

Related PR: citrineos/citrineos-operator-ui#78

  • feat: added certificateFileHash field to the Certificate record to maintain a unique hash for a cert in the DB to ensure that we can persist a matching unique record for each cert file that is uploaded into fileAccess
  • feat: created InstallCertificateAttempt and DeleteCertificateAttempt repositories to help keep track of certs that are being installed / deleted so that the InstalledCertificates repository can be updated appropriately when successful responses from the charger are received
  • feat: created UploadExistingCertificate endpoint to support being able to upload a certificate and tie it as an installed certificate on the charger, to be used during initial set up to upload certificates into COS that are already set up on the charger
  • feat: created RegenerateExistingCertificate to generate a new certificate that is signed by old
  • feat: adjusting InstallCertificate endpoint to get a certificate hash for the certificate that is being installed, and if a Certificate record does not yet exist for the provided certificate / hash, then a new record is created and the cert file is uploaded via fileAccess. The InstallCertificateAttempt record is created in pending state (status null) which will later be used to handle appropriately setting InstalledCertificate record
  • feat: adjusted InstallCertificate handler to check for and update applicable pending InstallCertificateAttempt record and if the InstallCertificate status was successful, then InstalledCertificate record will be created / updated appropriately
  • feat: adjusting GetGetInstalledCertificateIds handler to appropriately update InstalledCertificate records where previously we were deleting all and bulk inserting, now it will look for existing matches and update or insert new
  • feat: adjusting DeleteCertificate handler to appropriately update InstalledCertificate records
  • fix: CORS issue happening because OPTIONS was missing from list of supported methods

feat: added certificateFileHash field to the Certificate record to ma…
bf74116 
…intain a unique hash for a cert in the DB to ensure that we can persist a matching unique record for each cert file that is uploaded into fileAccess

feat: created InstallCertificateAttempt and DeleteCertificateAttempt repositories to help keep track of certs that are being installed / deleted so that the InstalledCertificates repository can be updated appropriately when successful responses from the charger are received
feat: created UploadExistingCertificate endpoint to support being able to upload a certificate and tie it as an installed certificate on the charger, to be used during initial set up to upload certificates into COS that are already set up on the charger
feat: created RegenerateExistingCertificate to generate a new certificate that is signed by old
feat: adjusting InstallCertificate endpoint to get a certificate hash for the certificate that is being installed, and if a Certificate record does not yet exist for the provided certificate / hash, then a new record is created and the cert file is uploaded via fileAccess. The InstallCertificateAttempt record is created in pending state (status null) which will later be used to handle appropriately setting InstalledCertificate record
feat: adjusted InstallCertificate handler to check for and update applicable pending InstallCertificateAttempt record and if the InstallCertificate status was successful, then InstalledCertificate record will be created / updated appropriately
feat: adjusting GetGetInstalledCertificateIds handler to appropriately update InstalledCertificate records where previously we were deleting all and bulk inserting, now it will look for existing matches and update or insert new
feat: adjusting DeleteCertificate handler to appropriately update InstalledCertificate records
fix: CORS issue happening because OPTIONS was missing from list of supported methods
@elliot-sabitov elliot-sabitov mentioned this pull request Nov 21, 2024
Open
Base automatically changed from rc-1.5.0 to main December 2, 2024 19:28
@thanaParis thanaParis changed the base branch from main to rc-1.6.0 December 2, 2024 19:58
lydiazcheng
lydiazcheng reviewed Dec 5, 2024
View reviewed changes
Copy link
Contributor

@lydiazcheng lydiazcheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Elliot, thanks for the effort! This code change is so big and complex. 👍 I think it would be better to test the related flows manually before merging if haven't.

01_Data/src/interfaces/repositories.ts
@@ -73,6 +74,7 @@ import { TariffQueryString } from './queries/Tariff';
import { LocalListVersion } from '../layers/sequelize/model/Authorization/LocalListVersion';
import { SendLocalList } from '../layers/sequelize/model/Authorization/SendLocalList';
import { InstalledCertificate } from '../layers/sequelize/model/Certificate/InstalledCertificate';
import { InstallCertificateAttempt } from '../layers/sequelize/model/Certificate/InstallCertificateAttempt';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super minor: InstallCertificateAttempt can be put together with DeleteCertificateAttempt at line 55.

01_Data/src/layers/sequelize/model/Certificate/Certificate.ts
@@ -63,6 +63,9 @@ export class Certificate extends Model {
@Column(DataType.STRING)
declare certificateFileId?: string | null;

@Column(DataType.STRING)
declare certificateFileHash?: string | null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be unique?

01_Data/src/layers/sequelize/model/Certificate/DeleteCertificateAttempt.ts
station?: ChargingStation;

@Column({
type: DataType.ENUM('SHA256', 'SHA384', 'SHA512'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just using values from the enum? The same idea for enum columns in InstallCertificateAttempt and InstalledCertificate .

Suggested change
type: DataType.ENUM('SHA256', 'SHA384', 'SHA512'),
type: DataType.ENUM(...Object.values(HashAlgorithmEnumType)),

01_Data/src/layers/sequelize/model/Certificate/InstallCertificateAttempt.ts
})
declare status?: InstallCertificateStatusEnumType | null;

declare customData?: CustomDataType | null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be missed to remove?

01_Data/src/layers/sequelize/model/Certificate/InstalledCertificate.ts
@@ -15,31 +16,38 @@ export class InstalledCertificate extends Model implements CertificateHashDataTy

@Column({
type: DataType.STRING,
allowNull: false,
allowNull: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does these column changes from required to optional?

03_Modules/Certificates/src/module/api.ts
const [newCertificatePem, _newPrivateKeyPem] = generateCertificate(
existingCertificateRecord,
this._logger,
undefined,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to use a existing cert to sign a new one, the issuerKeyPem is required. It should match the public key of the existingCertificate. We can find it by existingCertificateRecord.privateKeyFileId and then use this._fileAccess.getFile() to get the pem.

03_Modules/Certificates/src/module/api.ts
const existingCertificateBuffer = await this._fileAccess.getFile(fileId);
const existingCertificate = existingCertificateBuffer.toString();
const [newCertificatePem, _newPrivateKeyPem] = generateCertificate(
existingCertificateRecord,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we cannot use existingCertificateRecord directly. The new certificate will be generate based on the value that is assigned to certificateEntity of this generateCertificate() method. For instance, we need a new validBefore for new cert rather than the old one in existingCertificateRecord. The issuer and subject are also different.

How about we move the part between line 682 and 702 before generateCertificate and use that new certificate entity for the method?

03_Modules/Certificates/src/module/api.ts
const newCertificateHash = this.getCertificateHash(newCertificatePem);
let newCertificateRecord = new Certificate();
newCertificateRecord.serialNumber = moment().valueOf();
newCertificateRecord.issuerName = existingCertificateRecord.issuerName;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issuerName of the new cert should be the subjectName of the existing certificate.

03_Modules/Certificates/src/module/api.ts
existingCertificateRecord.organizationName;
newCertificateRecord.commonName = existingCertificateRecord.commonName;
newCertificateRecord.keyLength = existingCertificateRecord.keyLength;
newCertificateRecord.validBefore = existingCertificateRecord.validBefore;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a new date in validBefore. The existing one might be outdated.

03_Modules/Certificates/src/module/api.ts
newCertificateRecord.countryName = existingCertificateRecord.countryName;
newCertificateRecord.isCA = existingCertificateRecord.isCA;
newCertificateRecord.pathLen = existingCertificateRecord.pathLen;
newCertificateRecord.signedBy = existingCertificateRecord.signedBy;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

signedBy would be existingCertificateRecord.id

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lydiazcheng lydiazcheng lydiazcheng left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@elliot-sabitov @lydiazcheng