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

Deploy Gaia-X Authority did document #175

Merged
merged 28 commits into from
Jul 4, 2022

Conversation

Izzzu
Copy link

@Izzzu Izzzu commented Jun 29, 2022

Deployment of GAIA-X Authority DID in Deploy pipeline.
DID document contains public key in the same format as Registration Service did document.

Further notes

  • renaming the registry to either authority for did document resources and registrationservice for Registration Service resources for consistency
  • removing branches: [ main ] from CD workflow to enable checks in all PRs

linked to #151

@Izzzu Izzzu changed the base branch from feature/151-gaiax-did to main June 29, 2022 13:17
@Izzzu Izzzu changed the title Deploy GaiaX-Authority did Deploy Gaia-X Authority did document Jun 29, 2022
@Izzzu Izzzu changed the base branch from main to feature/151-gaiax-did June 29, 2022 13:59
@Izzzu Izzzu marked this pull request as ready for review June 29, 2022 14:00
.github/workflows/deploy.yaml Outdated Show resolved Hide resolved
.github/workflows/deploy.yaml Show resolved Hide resolved
.github/workflows/deploy.yaml Outdated Show resolved Hide resolved
.github/workflows/deploy.yaml Outdated Show resolved Hide resolved
deployment/terraform/dataspace/variables.tf Outdated Show resolved Hide resolved
Copy link

@chrislomonico chrislomonico left a comment

Choose a reason for hiding this comment

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

Please review comments provided

Copy link

@algattik algattik left a comment

Choose a reason for hiding this comment

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

The DID and private key are related not just to "registration service", but really to "dataspace authority" which is more than that. Names should reflect this

@Izzzu
Copy link
Author

Izzzu commented Jul 1, 2022

The DID and private key are related not just to "registration service", but really to "dataspace authority" which is more than that. Names should reflect this

indeed, I renamed the registry to either authority for did document resources and registrationservice for Registration Service resources

@@ -0,0 +1,18 @@
name: "Generate key"
description: "Generate key"
Copy link

Choose a reason for hiding this comment

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

could we extend this description


inputs:
keyname:
description: 'Name of the key file name'
Copy link

Choose a reason for hiding this comment

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

Suggested change
description: 'Name of the key file name'
description: 'Prefix for key file names'

runs:
using: "composite"
steps:
- name: 'Generate GAIA-X Authority key'
Copy link

Choose a reason for hiding this comment

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

needs generalisation

description: "Generate key"

inputs:
keyname:
Copy link

Choose a reason for hiding this comment

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

Suggested change
keyname:
keyFileNamePrefix:

- name: 'Generate Dataspace Authority key'
uses: ./.github/actions/generate-key
with:
keyname: registrationservicekey
Copy link

Choose a reason for hiding this comment

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

Suggested change
keyname: registrationservicekey
keyname: authoritykey

openssl ecparam -name prime256v1 -genkey -noout -out key.pem
openssl ec -in key.pem -pubout -out key.public.pem
docker run -i danedmunds/pem-to-jwk:1.2.1 --public --pretty < key.public.pem > key.public.jwk
- name: 'Generate Participant's key'
Copy link

Choose a reason for hiding this comment

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

Suggested change
- name: 'Generate Participant's key'
- name: 'Generate Participant key'

}
],
"verificationMethod" = [
{
"id" = "#identity-key-1"
"id" = "#identity-key-registration-service"
Copy link

Choose a reason for hiding this comment

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

Suggested change
"id" = "#identity-key-registration-service"
"id" = "#identity-key-authority"

Copy link

@chrislomonico chrislomonico left a comment

Choose a reason for hiding this comment

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

open questions/comments addressed

Copy link

@cpeeyush cpeeyush left a comment

Choose a reason for hiding this comment

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

Approved with suggestions which can be discussed later, no action needed for this PR at the moment. Thank you.

Comment on lines +38 to +39
variable "key_file_authority" {
description = "name of a file containing the Registration Service private key in PEM format"
Copy link

Choose a reason for hiding this comment

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

nit: By reading description and the variable name it gives the impression that authority and registration service are related as its usage registration service private key as per the description.

No action needed on this PR but we can discuss it later.

key_vault_id = azurerm_key_vault.registry.id
count = var.key_file_authority == null ? 0 : 1
value = file(var.key_file_authority)
key_vault_id = azurerm_key_vault.registrationservice.id
Copy link

Choose a reason for hiding this comment

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

nit: Same as below comment, If registration service does not need a key vault, then maybe we do not deploy one for that and rename to authority as currently in this PR registration service key vault is used to store authority did private key only.

No action for now can be discussed in a follow-up PR.

@Izzzu Izzzu merged commit 2827a86 into feature/151-gaiax-did Jul 4, 2022
@zeier zeier linked an issue Jul 7, 2022 that may be closed by this pull request
3 tasks
cpeeyush pushed a commit that referenced this pull request Jul 11, 2022
* Deploy Gaia-X Authority did document (#175)

* Rename 'registry' to 'registration service' consistently (#196)
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.

GAIA-X Authority - DID
4 participants