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

Adds 'TEST of KLASS3-SK 2016' certificate and organization certs support #7

Closed
wants to merge 2 commits into from

Conversation

erkkiarus
Copy link
Contributor

  • Added TEST of KLASS3-SK 2016 to trusted CA list;
  • Uses Common Name in case of an organization certificate instead of first and last name.

Signed-off-by: Erkki Arus erkki@raulwalter.com

* Added TEST of KLASS3-SK 2016 to trusted CA list;
* Uses Common Name in case of an organisation certificate instead of
first and last name;
* Merged with current main branch.

Signed-off-by: Erkki Arus <erkki@raulwalter.com>
@@ -10,6 +10,14 @@
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
</PropertyGroup>

<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|AnyCPU'">
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 you should set x64 target. For best compatibility

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the best compatibility it should be AnyCPU while in that case the architecture stays unspecified in IL and it is able to run on any target platform. x64 limits it with 64-bit platforms.

Copy link
Contributor

@metsma metsma Apr 27, 2024

Choose a reason for hiding this comment

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

We only provide x64 binaries for libdigidocpp. I cannot sign anything under arm64 windows and ubuntu

Replaced AnyCPU configuration with x64 as target platform while libdigidocpp
is provided as x64 binaries only.

Signed-off-by: Erkki Arus <erkki@raulwalter.com>
@@ -13,7 +13,7 @@ RUN echo "deb [signed-by=/usr/share/keyrings/ria-repository.gpg] https://install
apt-get clean && \
rm -rf /var/lib/apt/lists

COPY ./WebEid.AspNetCore.Example/bin/Release/net6.0/publish/ .
COPY ./WebEid.AspNetCore.Example/bin/x64/Release/net6.0/publish/ .
Copy link
Contributor

@rabadashTheFool rabadashTheFool May 8, 2024

Choose a reason for hiding this comment

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

Is there any reason why we cannot use AnyCPU and have to use the x64 build?

Same for the .sln file changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Like I said, RIA provides only x64 binaries on windows and ubuntu.
Most mac developers have arm64 computers, therefore they cannot run/debug/develop the application on windows and ubuntu.

@@ -60,7 +86,7 @@ dotnet build

If you have a test eID card, use the `Development` profile. In this case access to paid services is not required, but you need to upload the authentication and signing certificates of the test card to the test OCSP responder database as described in section _[Using DigiDoc4j in test mode with the `dev` profile](https://github.com/web-eid/web-eid-spring-boot-example#using-digidoc4j-in-test-mode-with-the-dev-profile)_ of the Web eID Java example application documentation. The`Development` profile is activated by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to use an organization certificate in a Development mode? Similarly to the test ID-card.

}
catch (ArgumentOutOfRangeException)
{
logger.LogWarning("Claim {0} not presented", claimGetter.Key);
Copy link
Contributor

@rabadashTheFool rabadashTheFool May 8, 2024

Choose a reason for hiding this comment

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

When using an organization certificate, we would not want to log missing fields which we were not expecting in the first place.
As such, Logger is not yet necessary in this file.

string claimData = claimGetter.Value.Invoke();
claims.Add(new Claim(claimGetter.Key, claimData));
}
catch (ArgumentOutOfRangeException)
Copy link
Contributor

@rabadashTheFool rabadashTheFool May 8, 2024

Choose a reason for hiding this comment

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

When creating a new Claim, the exception thrown is ArgumentNullException when either the type or value is null.
Furthermore, we should not try to add Claims with missing values but rather skip them.

var claims = new List<Claim>
var certificate = await authTokenValidator.Validate(authToken.AuthToken, challengeNonceStore.GetAndRemove().Base64EncodedNonce);

Dictionary<string, Func<string>> claimDataGetters = new()
Copy link
Contributor

@rabadashTheFool rabadashTheFool May 8, 2024

Choose a reason for hiding this comment

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

Since Dictionary is unordered and Claims in ClaimsIdentity are ordered, we should try to maintain that order.
Instead of that, we could introduce a helper method for adding new claims to claims list, such as:
AddNewClaimIfCertificateHasData(List claims, string claimType, Func dataGetter)

This would also make the code more readable and remove the necessity of creating new temporary dictionaries and loops.

Copy link
Contributor

@rabadashTheFool rabadashTheFool left a comment

Choose a reason for hiding this comment

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

There are some questionable changes within the pull request.
Both in regards to changes in solution file, launchSettings and the business logic for the example.

@metsma
Copy link
Contributor

metsma commented May 9, 2024

There are some questionable changes within the pull request. Both in regards to changes in solution file, launchSettings and the business logic for the example.

launchSettings changes are generated by visual studio

@mrts
Copy link
Member

mrts commented May 17, 2024

This will be superseded by #21.

@mrts
Copy link
Member

mrts commented Jun 21, 2024

Closing in favor of #21.

Your work was included there, thanks for your contribution!

@mrts mrts closed this Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants