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.

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

X509 certificate provision #7935

Conversation

AndriiLandiak
Copy link
Member

@AndriiLandiak AndriiLandiak commented Jan 12, 2023

Pull Request description

Related issues: #6735, #7668

UI changes:
Device profile entity -> Device provisioning -> component DeviceProfileProvisionConfigurationComponent -> added form controls for provision strategy "X509 Certificates Chain".

Screenshots:
image
image
image
image
image
image

General checklist

  • You have reviewed the guidelines document.
  • PR name contains fix version. For example, "[3.3.4] Hotfix of some UI component" or "[3.4] New Super Feature".
  • The milestone is specified and corresponds to fix version.
  • Description references specific issue.
  • Description contains human-readable scope of changes.
  • Description contains brief notes about what needs to be added to the documentation.
  • No merge conflicts, commented blocks of code, code formatting issues.
  • Changes are backward compatible or upgrade script is provided.
  • Similar PR is opened for PE version to simplify merge. Required for internal contributors only.

Back-End feature checklist

  • Added corresponding unit and/or integration test(s). Provide written explanation in the PR description if you have failed to add tests.
  • If new dependency was added: the dependency tree is checked for conflicts.
  • If new service was added: the service is marked with corresponding @TbCoreComponent, @TbRuleEngineComponent, @TbTransportComponent, etc.
  • If new REST API was added: the RestClient.java was updated, issue for Python REST client is created.

Front-End feature checklist

  • Screenshots with affected component(s) are added. The best option is to provide 2 screens: before and after changes;
  • If you change the widget or other API, ensure it is backward-compatible or upgrade script is present.
  • Ensure new API is documented here

String deviceCommonName = "";
try {
deviceCommonName = SslUtil.parseCommonName(readCertFile(chain.get(0)));
} catch (Exception ignored) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you prefer to ignore this exception?
I believe in real case production that includes a lot of potential points of failure, it's important quickly understand where is the root cause of the problem and warn message in the core engine that certificate cannot be parsed or common name cannot be obtain could help on this.

Copy link
Member Author

@AndriiLandiak AndriiLandiak Mar 28, 2023

Choose a reason for hiding this comment

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

I realize that it's not obvious, but this error occurs if we cannot read and create an X509 object from a certain String representation (I mean readCertFIle in such case from chain.get(0)), so I am not sure it's the right warning message. But I'll provide an additional warning in the method parseCommonName if we cannot do it.

@AndriiLandiak AndriiLandiak changed the title [WIP][3.5] X509 device certificate improvements [3.5] X509 device certificate improvements Mar 29, 2023
@AndriiLandiak AndriiLandiak changed the title [3.5] X509 device certificate improvements [3.5] X509 certificate provision Mar 30, 2023
application/src/main/data/upgrade/3.4.4/schema_update.sql Outdated Show resolved Hide resolved
@@ -180,6 +180,10 @@ message ValidateDeviceX509CertRequestMsg {
string hash = 1;
}

message ValidateOrCreateDeviceX509CertRequestMsg {
Copy link
Member

Choose a reason for hiding this comment

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

Could we just add a field to ProvisionDeviceCredentialsMsg?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. This is not a classic provision strategy, in most cases, it will work as before - connect to devices, so I believe that sending CredentialsDataProto with empty fields to connect is a bad approach.

@vvlladd28 vvlladd28 added UI UI changes feature Core Changes to Core labels Apr 6, 2023
@ashvayka ashvayka changed the title [3.5] X509 certificate provision X509 certificate provision Apr 7, 2023
@ashvayka ashvayka added this to the 3.5 milestone Apr 7, 2023
@ashvayka ashvayka removed the UI UI changes label Apr 7, 2023
@ashvayka ashvayka added Major Core Major improvement to Core Services and removed feature Core Changes to Core labels Apr 7, 2023
@AndriiLandiak AndriiLandiak changed the base branch from develop/3.5 to feature/x509-device-provisioning April 12, 2023 14:51
@AndriiLandiak AndriiLandiak changed the base branch from feature/x509-device-provisioning to develop/3.5 April 12, 2023 15:05
@AndriiLandiak AndriiLandiak changed the base branch from develop/3.5 to feature/x509-device-provisioning April 12, 2023 15:12
@ashvayka ashvayka merged commit e7f7440 into thingsboard:feature/x509-device-provisioning Apr 13, 2023
@AndriiLandiak AndriiLandiak deleted the feature/x509-device-cert-impr branch April 13, 2023 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Major Core Major improvement to Core Services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants