minvws / nl-covid19-notification-app-backend Goto Github PK

Describe the bug, issue or concern

The concern is the existence of the code block in https://github.com/minvws/nl-covid19-notification-app-backend/blob/master/Components/Icc/Models/TheIdentityHubClaimTypes.cs:

        internal const string AccessToken = "http://schemas.u2uconsult.com/ws/2014/03/identity/claims/accesstoken";
        internal const string AuthenticationStrength = "http://schemas.u2uconsult.com/ws/2016/08/identity/claims/authenticationstrength";
        internal const string ClientId = "http://schemas.u2uconsult.com/ws/2014/11/identity/claims/clientid";
        internal const string OldIdentityId = "http://schemas.u2uconsult.com/ws/2019/02/identity/claims/oldidentityid";
        internal const string DisplayName = "http://schemas.u2uconsult.com/ws/2014/04/identity/claims/displayname";
        internal const string EmailAddress = "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress";
        internal const string EmailAddressVerified = "http://schemas.u2uconsult.com/ws/2017/02/identity/claims/emailaddressverified";
        internal const string GivenName = "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/givenname";
        internal const string IdentityProvider = "http://schemas.microsoft.com/accesscontrolservice/2010/07/claims/identityprovider";
        internal const string LargePicture = "http://schemas.u2uconsult.com/ws/2014/04/identity/claims/largepicture";
        internal const string MediumPicture = "http://schemas.u2uconsult.com/ws/2014/04/identity/claims/mediumpicture";
        internal const string Scope = "http://schemas.u2uconsult.com/ws/2014/03/identity/claims/scope";
        internal const string SmallPicture = "http://schemas.u2uconsult.com/ws/2014/04/identity/claims/smallpicture";
        internal const string Surname = "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/surname";
        internal static readonly string NameIdentifier = "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/nameidentifier";

I'm assuming these are either not used or they are used to indicate URLs to third-parties that can be used to establish the validity of the schemes. However, I haven't looked into it. Reasons to remove this:

• These are SEO links for Google to boost the http://u2uconsult.com site (this one as well :-) ).
• These are http links rather than https links, so vulnerable for man in the middle attacks.
• The corresponding server get hits from the system verifying their schemas. This leaks information on use.
• Most of those links do not currently resolve.

Expected behavior

Fix. Https links to your own backend or remove.

Governance

• I've read the contributing document https://github.com/minvws/.github/blob/master/CONTRIBUTING.md
• I've read and understand the Code of Conduct https://github.com/minvws/.github/blob/master/CODE_OF_CONDUCT.md
• I understand that any contributions or suggestions I made may make it into the actual code. I've read the License https://github.com/minvws/nl-covid19-notification-app-coordination/blob/master/LICENSE.txt and the contributor license agreement https://github.com/minvws/nl-covid19-notification-app-coordination/blob/master/CLA.md

Describe the bug, issue or concern

Docker local run fails

To Reproduce

Steps to reproduce the behavior:

1. git clone repo
2. cd docker
3. docker-compose up --build
4. Errors in Dockerfile: due deletion of Components/Framework/WindowsIdentityStuff.cs on initial run
   solved by using RUN rm -rf
   issues with the destination of csproj files and new names given to projects (CotentApi & MobileAppApi), path should be prefixed with src/

Missing projects:
DbProvision/DbProvision.csproj
DbFillExampleContent/DbFillExampleContent.csproj

Changed names of exe files in entrypoint in docker-compose.yml

Expected behavior

instruction from readme to run the project in docker should work without errors

Governance

• [ X] I've read the contributing document https://github.com/minvws/.github/blob/master/CONTRIBUTING.md
• [ X] I've read and understand the Code of Conduct https://github.com/minvws/.github/blob/master/CODE_OF_CONDUCT.md
• [ X] I understand that any contributions or suggestions I made may make it into the actual code. I've read the License https://github.com/minvws/nl-covid19-notification-app-coordination/blob/master/LICENSE.txt and the contributor license agreement https://github.com/minvws/nl-covid19-notification-app-coordination/blob/master/CLA.md

Concern

I see that WireMock.Net is used to unit-test the TheIdentityHubService in the TheIdentityHubServiceTests.cs file.

However a fixed port is used:

This can cause issues when running these unit-tests on a build-server, there is not 100% guarantee that this port will be free on the OS.

My suggestion would be to start the WireMock.Net without a fixed port. Just like:

var server = WireMockServer.Start();

// Getting the random port on which the WireMock.Net server is running, can be done like:
var port = server.Ports[0];

// Or you can also get the full URL where the WireMock.Net server is running with:
var url = server.Urls[0];

However this means that setting up the TheIdentityHubOptions should be done in each unit-test because the port is not fixed anymore.

Another suggestion:
I see that in every test the WireMock.Net server is started and stopped, in that case using a local variable var server = instead of defining and reusing a private class variable like private WireMockServer _Server; would be a better option.

Another idea would be to start the WireMock.Net server once during start from the test-class and reset + add a new mapping in each test. This can be a bit quicker when running the tests.

It hurts when the name of the world's most awesomest database is mangled like this.

What is this project doing?
I see it mentions .NET, MSSQL, as Node JS, Yarn and Angular CLI, shouldn't the 'frontend' for care workers be in another project?

My advise to make this more accessible for other developer 👀

• Update README.md with folder overview plus short summary
• Put README.md in every folder with the project/folder description what this thing is doing.

I don't think you want your private key visible like that. Best remove it quickly and regenerate.
It gives me the creeps to see stuff like this in a code base.

Also, I see you're not working with a process to handle security issues like this one.
Would be nice to have that in place too.

Describe the bug, issue or concern

Currently docker-compose up --build fails on a clean clone due to a couple of errors in the Docker file.

This is running on Ubuntu 20.04.

To Reproduce

Steps to reproduce the behavior:

1. Create a fresh clone of the repository:

git clone https://github.com/minvws/nl-covid19-notification-app-backend.git

2. Enter the freshly created repository.

cd nl-covid19-notification-app-backend

3. Start the docker compose process as described in the docs.

cd docker
docker-compose up --build

4. Generates the following output and error.

$ docker-compose up --build
Building mobile_api
Step 1/23 : FROM mcr.microsoft.com/dotnet/core/sdk:3.1-alpine AS builder
 ---> 656495ef4e20
Step 2/23 : COPY . app/
 ---> Using cache
 ---> c642901eb776
Step 3/23 : WORKDIR app/
 ---> Using cache
 ---> 1ace1d52bf4c
Step 4/23 : COPY docker/development/appsettings.json .
 ---> Using cache
 ---> 88648b988891
Step 5/23 : RUN rm Components/Framework/WindowsIdentityStuff.cs
 ---> Running in d0d719382673
rm: can't remove 'Components/Framework/WindowsIdentityStuff.cs': No such file or directory
ERROR: Service 'mobile_api' failed to build: The command '/bin/sh -c rm Components/Framework/WindowsIdentityStuff.cs' returned a non-zero code: 1

5. Fixing this error generates further output leading to another error:

MSBUILD : error MSB1009: Project file does not exist.
Switch: DbFillExampleContent/DbFillExampleContent.csproj
ERROR: Service 'mobile_api' failed to build: The command '/bin/sh -c dotnet publish DbFillExampleContent/DbFillExampleContent.csproj --no-self-contained  --configuration Release -o publish/Tools/DbFillExampleContent --version-suffix local' returned a non-zero code: 1

Expected behavior

The docker containers are successfully brought up.

Screenshots

n/a, see console output above

Additional context

I'll shortly submit a simple PR to address the issues.

Governance

• [ x ] I've read the contributing document https://github.com/minvws/.github/blob/master/CONTRIBUTING.md
• [ x ] I've read and understand the Code of Conduct https://github.com/minvws/.github/blob/master/CODE_OF_CONDUCT.md
• [ x ] I understand that any contributions or suggestions I made may make it into the actual code. I've read the License https://github.com/minvws/nl-covid19-notification-app-coordination/blob/master/LICENSE.txt and the contributor license agreement https://github.com/minvws/nl-covid19-notification-app-coordination/blob/master/CLA.md

https://github.com/minvws/nl-covid19-notification-app-backend/blob/ba0938c3b37c9debed16de80f4206f45033d7021/Components/Services/Signing/HardCodedExposureKeySetSigning.cs

First of all I would like to say that I really appreciate that this work is being open sourced.

I want to propose adding a security.txt file to be publicly served by this application.
security.txt is a proposed standard for listing responsible disclosure policies and contact information. Its purpose is to provide a standard location so security researchers can easily get in contact to report a security issue. The standard is currently in draft stage but is already in use by organizations like Google and GitHub.

This GitHub repository already has a security policy, but once this application is running in production a security researcher who has found a vulnerability might not be aware of the fact that this repository is publicly visible. They will have to search for out-of-band information in order to responsibly disclose security issues. As explained for example by security researcher Troy Hunt, who runs the popular data breach notification service Have I Been Pwned?, the security.txt file is one of the first things he will look for when he tries to contact an organization about a data breach.
Given the high public profile of this application and the focus on privacy and security I believe it is a good step to serve this file, and it comes at no real cost.

Based on the existing security policy published on GitHub, the contents of security.txt could look like this:

Contact: https://www.ncsc.nl/contact/kwetsbaarheid-melden
Contact: https://english.ncsc.nl/contact/reporting-a-vulnerability-cvd
Encryption: https://english.ncsc.nl/contact/pgp-key
Policy: https://github.com/minvws/nl-covid19-notification-app-backend/security/policy

This file should be served from /.well-known/security.txt (RFC8615). Other fields, such as Acknowledgments, Canonical and Preferred-Languages are also supported. Please refer to the draft standard or to https://securitytxt.org/ for the full specification.

I would be happy to create a pull request, but I'm not sure where to create this file in order for it to be served as a static file in the document root. If I'm understanding correctly the Docker configuration and ServerStandAlone directory are only used for development, and will not be used in production. Alternatively the file could be stored in the root of the repository (as suggested on https://securitytxt.org/) and be be copied to the right location during build-time. As I said, if someone can point me in the right direction I would happily create a pull request for this.

Describe the bug, issue or concern

When running the docker-compose install on Linux, executing the following command fails:

docker exec docker_content_api_1 EksEngine/EksEngine

This is running on Ubuntu 20.04.

To Reproduce

Steps to reproduce the behavior:

1. Start/build the docker install on Linux.

docker-compose up --build

2. Initialise the database.

docker exec docker_content_api_1 Tools/DbProvision/DbProvision

3. Generate Teks.

docker exec docker_content_api_1 Tools/GenTeks/GenTeks
docker exec docker_content_api_1 Tools/ForceTekAuth/ForceTekAuth

4. Attempt to initialise the Eks engine

docker exec docker_content_api_1 EksEngine/EksEngine

5. Generates the following error.

[21:38:59 FTL] System.PlatformNotSupportedException: Windows Principal functionality is not supported on this platform.
   at System.Security.Principal.WindowsIdentity.GetCurrent()
   at NL.Rijksoverheid.ExposureNotification.BackEnd.Components.Framework.WindowsIdentityQueries.CurrentUserIsAdministrator() in /app/Components/Framework/WindowsIdentityQueries.cs:line 13
   at NL.Rijksoverheid.ExposureNotification.BackEnd.Components.ExposureKeySetsEngine.ExposureKeySetBatchJobMk3.Execute() in /app/Components/ExposureKeySetsEngine/ExposureKeySetBatchJobMk3.cs:line 78
   at NL.Rijksoverheid.ExposureNotification.BackEnd.EksEngine.Program.Start(IServiceProvider serviceProvider, String[] args) in /app/EksEngine/Program.cs:line 46
   at NL.Rijksoverheid.ExposureNotification.BackEnd.Components.ConsoleApps.ConsoleAppRunner.Execute(String[] args, Action`2 configure, Action`2 start) in /app/Components/ConsoleApps/ConsoleAppRunner.cs:line 41

6. Notice that there are no exposureKeySet ids in the manifest.

Expected behavior

The EksEngine starts up successfully and the exposureKeySet ids are added to the manifest.

Screenshots

n/a.

Additional context

PR #27 highlights where the errors occur in the EtkEngine code. The problem stems from the fact that the user certificate store isn't available on Linux (I'm not sure whether this is a Linux issue or a Docker issue though). The PR works around the problem, but isn't really a fix because it results in dummy signatures being generated (it does, however, make clear where the issues are, I think).

Governance

• [ x ] I've read the contributing document https://github.com/minvws/.github/blob/master/CONTRIBUTING.md
• [ x ] I've read and understand the Code of Conduct https://github.com/minvws/.github/blob/master/CODE_OF_CONDUCT.md
• [ x ] I understand that any contributions or suggestions I made may make it into the actual code. I've read the License https://github.com/minvws/nl-covid19-notification-app-coordination/blob/master/LICENSE.txt and the contributor license agreement https://github.com/minvws/nl-covid19-notification-app-coordination/blob/master/CLA.md

Describe the bug, issue or concern

According to the design documentation the decoy probability should change depending on positive test numbers (and possibly install counts). However, it seems to be fixed to 0.00118, also on the production CDN.

Governance

• I've read the contributing document https://github.com/minvws/.github/blob/master/CONTRIBUTING.md
• I've read and understand the Code of Conduct https://github.com/minvws/.github/blob/master/CODE_OF_CONDUCT.md
• I understand that any contributions or suggestions I made may make it into the actual code. I've read the License https://github.com/minvws/nl-covid19-notification-app-coordination/blob/master/LICENSE.txt and the contributor license agreement https://github.com/minvws/nl-covid19-notification-app-coordination/blob/master/CLA.md

Moved to minvws/nl-covid19-notification-app-coordination#66

Some files in this repository contain secrets. I don't think you want to expose sa db - passwords to the whole world.

For instance here and here. (You should also not add an appsettings.development.json file in source-control).

Make use of docker secrets to inject secrets in a container; do not expose connectionstrings via the git repository!