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

Npatilsen/dps cert march #1687

Draft
wants to merge 16 commits into
base: preview
Choose a base branch
from
Draft

Npatilsen/dps cert march #1687

wants to merge 16 commits into from

Conversation

patilsnr
Copy link
Contributor

@patilsnr patilsnr commented Mar 7, 2023 •
edited
Loading

No description provided.

@patilsnr patilsnr changed the base branch from main to preview April 11, 2023 18:18
@timtay-microsoft
Copy link
Member

Abhipsa had to upgrade the service API version to "2021-11-01-preview" FYI. You'll find this hardcoded in SDKUtils.java I think

abhipsaMisra
abhipsaMisra reviewed Apr 25, 2023
View reviewed changes
...ice-client/src/main/java/com/microsoft/azure/sdk/iot/provisioning/device/AdditionalData.java
*/
@Getter
@Setter
public String operationalCertificateRequest;
Copy link
Member

Choose a reason for hiding this comment

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

The choice of variable name on the .NET SDK (we'd like to keep the variable name same across all languages so as to keep the user experience similar): https://github.com/Azure/azure-iot-sdk-csharp/blob/c3008205c68de5b8fede085533048987e15af1c9/provisioning/device/src/ProvisioningRegistrationAdditionalData.cs#L23

patilsnr
patilsnr commented May 15, 2023
View reviewed changes
...ample/src/main/java/samples/com/microsoft/azure/sdk/iot/ServiceLinkEnrollmentToCASample.java
@@ -0,0 +1,47 @@
// Copyright (c) Microsoft. All rights reserved.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was completing step ii of the .NET implementation of this feature and though that it could be useful to make it a very small sample. My feelings on it are mixed since its presence without understanding its relationship to the main feature sample in the device client could cause confusion. I've added it into the pom.xml for the time being and would love feedback on whether doing this step via a sample like this or just leaving it to the user would be better.

timtay-microsoft
timtay-microsoft reviewed May 15, 2023
View reviewed changes
...src/main/java/samples/com/microsoft/azure/sdk/iot/ProvisioningCertificateIssuanceSample.java
@@ -9,20 +9,22 @@
import com.microsoft.azure.sdk.iot.provisioning.device.AdditionalData;
import com.microsoft.azure.sdk.iot.provisioning.device.internal.exceptions.ProvisioningDeviceClientException;
import com.microsoft.azure.sdk.iot.provisioning.security.SecurityProvider;
//import com.microsoft.azure.sdk.iot.provisioning.service;
Copy link
Member

Choose a reason for hiding this comment

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

remove?

timtay-microsoft
timtay-microsoft reviewed May 15, 2023
View reviewed changes
...src/main/java/samples/com/microsoft/azure/sdk/iot/ProvisioningCertificateIssuanceSample.java
private static final String globalEndpoint = "[Your Provisioning Service Global Endpoint here]";
private static final ProvisioningDeviceClientTransportProtocol PROVISIONING_DEVICE_CLIENT_TRANSPORT_PROTOCOL = ProvisioningDeviceClientTransportProtocol.HTTPS;
private static final String idScope = "";
private static final String globalEndpoint = "";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private static final String globalEndpoint = "";
private static final String globalEndpoint = "global.azure-devices-provisioning.net";

This default value works for everyone who isn't on a govt. or private cloud instance, so we may as well fill it in for them

timtay-microsoft
timtay-microsoft reviewed May 15, 2023
View reviewed changes
...src/main/java/samples/com/microsoft/azure/sdk/iot/ProvisioningCertificateIssuanceSample.java
Comment on lines +113 to +114
System.out.println("Failed to create directory for certificates.");
throw new IOException();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
System.out.println("Failed to create directory for certificates.");
throw new IOException();
throw new IOException("Failed to create directory for certificates.");

timtay-microsoft
timtay-microsoft reviewed May 15, 2023
View reviewed changes
...ample/src/main/java/samples/com/microsoft/azure/sdk/iot/ServiceLinkEnrollmentToCASample.java
* Link Individual Enrollment in DPS to a Certificate Authority (CA). Intended to be used with the
* Provisioning Certificate Issuance Sample.
*/
public class ServiceLinkEnrollmentToCASample
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just "LinkEnrollmentToCASample" since "service" is implied by being in a provisioning service client sample folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd agree, but by that logic, all of the samples in the same folder would probably want to be renamed (which I'm not opposed to)
image

timtay-microsoft
timtay-microsoft reviewed May 15, 2023
View reviewed changes
...ample/src/main/java/samples/com/microsoft/azure/sdk/iot/ServiceLinkEnrollmentToCASample.java
Comment on lines +19 to +21
/*
* Details of the Provisioning.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/*
* Details of the Provisioning.
*/
// The connection string for your DPS instance.

timtay-microsoft
timtay-microsoft reviewed May 15, 2023
View reviewed changes
...om/microsoft/azure/sdk/iot/provisioning/service/configs/ClientCertificateIssuancePolicy.java
@@ -6,10 +6,10 @@
import lombok.Getter;
import lombok.Setter;

/* Custom class for issuing client certificates. */
/** Custom class for issuing client certificates. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** Custom class for issuing client certificates. */
/**
* Custom class for issuing client certificates.
*/

Copy link
Member

Choose a reason for hiding this comment

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

Same change below. Making this style of a comment a one liner is a bit awkward.

abhipsaMisra
abhipsaMisra reviewed May 15, 2023
View reviewed changes
...rc/main/java/com/microsoft/azure/sdk/iot/provisioning/device/internal/task/RegisterTask.java
@@ -101,7 +101,8 @@ public void run(ResponseData responseData, Object context) throws ProvisioningDe
throw new ProvisioningDeviceClientException(new IllegalArgumentException("authorization cannot be null"));
}

//SRS_RegisterTask_25_001: [ Constructor shall save provisioningDeviceClientConfig , securityProvider, provisioningDeviceClientContract and authorization.]
// check if using operational CSR
this.operationalClientCertificateRequest = provisioningDeviceClientConfig.getOperationalCertificateRequest();
Copy link
Member

Choose a reason for hiding this comment

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

The comment above references some checking happening here, but the code doesn't have any checks in this part of the implementation. Is that pending, or is the comment out-of-date?

abhipsaMisra
abhipsaMisra reviewed May 15, 2023
View reviewed changes
...rc/main/java/com/microsoft/azure/sdk/iot/provisioning/device/internal/task/RegisterTask.java
@@ -101,7 +101,8 @@ public void run(ResponseData responseData, Object context) throws ProvisioningDe
throw new ProvisioningDeviceClientException(new IllegalArgumentException("authorization cannot be null"));
}

//SRS_RegisterTask_25_001: [ Constructor shall save provisioningDeviceClientConfig , securityProvider, provisioningDeviceClientContract and authorization.]
// check if using operational CSR
this.operationalClientCertificateRequest = provisioningDeviceClientConfig.getOperationalCertificateRequest();
Copy link
Member

Choose a reason for hiding this comment

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

I don't see the operational certificate being set from the additional data into the provisioning client config. Am I missing something?

It seems like the field should be set in here:

azure-iot-sdk-java/provisioning/provisioning-device-client/src/main/java/com/microsoft/azure/sdk/iot/provisioning/device/ProvisioningDeviceClient.java

Lines 110 to 124 in 214a540

public void registerDevice(ProvisioningDeviceClientRegistrationCallback provisioningDeviceClientRegistrationCallback, Object context, AdditionalData additionalData) throws ProvisioningDeviceClientException
{
if (provisioningDeviceClientRegistrationCallback == null)
{
throw new IllegalArgumentException("registration callback cannot be null");
}
this.provisioningDeviceClientConfig.setPayload(additionalData.getProvisioningPayload());
this.provisioningDeviceClientConfig.setRegistrationCallback(provisioningDeviceClientRegistrationCallback, context);
log.debug("Starting provisioning thread...");
ProvisioningTask provisioningTask = new ProvisioningTask(this.provisioningDeviceClientConfig, this.provisioningDeviceClientContract);
executor.submit(provisioningTask);
}

abhipsaMisra
abhipsaMisra reviewed May 15, 2023
View reviewed changes
...rc/main/java/com/microsoft/azure/sdk/iot/provisioning/device/internal/task/RegisterTask.java
@@ -47,6 +47,7 @@ class RegisterTask implements Callable<RegistrationOperationStatusParser>
private final Authorization authorization;
private final SecurityProvider securityProvider;
private final ProvisioningDeviceClientConfig provisioningDeviceClientConfig;
private final String operationalClientCertificateRequest;
Copy link
Member

Choose a reason for hiding this comment

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

This field isn't being referenced anywhere in this class. Is this required?

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

@timtay-microsoft timtay-microsoft timtay-microsoft left review comments

@abhipsaMisra abhipsaMisra abhipsaMisra left review comments

@andyk-ms andyk-ms Awaiting requested review from andyk-ms andyk-ms will be requested when the pull request is marked ready for review andyk-ms is a code owner

@brycewang-microsoft brycewang-microsoft Awaiting requested review from brycewang-microsoft brycewang-microsoft will be requested when the pull request is marked ready for review brycewang-microsoft is a code owner

@tmahmood-microsoft tmahmood-microsoft Awaiting requested review from tmahmood-microsoft tmahmood-microsoft will be requested when the pull request is marked ready for review tmahmood-microsoft is a code owner

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.
3 participants
@patilsnr @timtay-microsoft @abhipsaMisra