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

refactor(cmd-api-server): clean up configuration parameters #720 #1996

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

ruzell22
Copy link
Contributor

fixes: #720

Parameters that are cleaned up are: cactusNodeId, consortiumId, keychainSuffixKeyPairPem
Parameter keyPairPem cannot be remove as it results to an error in running the api server.

Cleaning the three mentioned parameter are backwards compatible with tags versions:
v1.0.0-rc.3 and v1.0.0
The latest tag being used as of this change is v1.0.0-25-gdda3f00c

Signed-off-by: ruzell22 [email protected]

@petermetz petermetz removed the request for review from jonathan-m-hamilton April 29, 2022 17:07
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@ruzell22 The documentation doesn't seem to be updated. Please go through the acceptance criteria again.

@ruzell22
Copy link
Contributor Author

@petermetz I have looked into the package cactus-cmd-api-server about the parameters to see if it is mentioned there on other files and for possible documentations. I have cleaned those that are not inside the other packages:
Screenshot 2022-05-11 133400 (changes 1)
Screenshot 2022-05-11 133418 (changes 2 and 3 whitepaper)

I have also ran npm run configure and npm run start:api-server command and it outputs a success
Screenshot 2022-05-11 133815 (npm run configure)
Screenshot 2022-05-11 133931 (npm run start api-server)

I would like to inquire if this is good to push for a PR or is there other specific documentation file I should update? Thank you

@ruzell22
Copy link
Contributor Author

Remove ENV CACTUS_NODE_ID and CONSORTIUM_ID in dockerfile under cactus-cmd-api-server package, KEYCHAIN_SUFFIX_KEY_PAIR_PEM was not mentioned in the docker file so there is none in the file changes in dockerfile.
npm run configure and npm run start:api-server command were also successful after the changes.
Screenshot 2022-05-12 172129 (npm run configure)
Screenshot 2022-05-12 172213 (npm run start api-server)

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@ruzell22 I'm seeing the variable names in free text search popping up several times throughout the code-base still.

@gitguardian
Copy link

gitguardian bot commented May 16, 2022

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id Secret Commit Filename
- Generic Private Key 0c0952b examples/cactus-example-carbon-accounting-backend/example-config.json View secret
- RSA Private Key 0c0952b examples/cactus-example-carbon-accounting-backend/example-config.json View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

@ruzell22 ruzell22 force-pushed the cmdapiservercleanup720 branch 2 times, most recently from a341ebe to 44f38ee Compare May 18, 2022 02:45
@ruzell22
Copy link
Contributor Author

@petermetz I have removed more cactusNodeId, consortiumId, and keychainSuffixKeyPairPem and their ENV equivalent (excluding in example-config.json file which causes GitGuardian bot error during checks here).

@ruzell22 ruzell22 requested a review from petermetz May 18, 2022 10:31
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@petermetz I have removed more cactusNodeId, consortiumId, and keychainSuffixKeyPairPem and their ENV equivalent (excluding in example-config.json file which causes GitGuardian bot error during checks here).

@ruzell22 If changing example-config.json trips the GitGuardian check we need to configure it to ignore that file because the alternative would be to not change that file ever again which is not feasable. Ignoring it in gitguardian should be safe because it's an example file with randomly generated keys in it.

@jagpreetsinghsasan
Copy link
Contributor

@ruzell22 any updates on this PR? (If you think the necessary changes were done, you can always re-request for review)

@ruzell22
Copy link
Contributor Author

ruzell22 commented Apr 6, 2023

Hello @petermetz , I have pushed the changes for the clean up so the new gitguardian workflow will be able to scan it. It passed. However, when I looked into more details about it, the scanner is still saying "Invalid token header. No credentials provided". It seems that even after being merged to the main repository, the workflow still doesn't have the permission to access the environmental secrets variable. I would like to ask assistance in that matter. Thank you. Below are the screenshots of the details.
image
image

@petermetz
Copy link
Contributor

Hello @petermetz , I have pushed the changes for the clean up so the new gitguardian workflow will be able to scan it. It passed. However, when I looked into more details about it, the scanner is still saying "Invalid token header. No credentials provided". It seems that even after being merged to the main repository, the workflow still doesn't have the permission to access the environmental secrets variable. I would like to ask assistance in that matter. Thank you. Below are the screenshots of the details. image image

@ruzell22 That's unfortunate. Please re-open the issue for the git guardian check, link to the comment you just made in a comment on that issue so that everyone has the context.
Then start a new PR to fix the issue that you re-opened.
Make sure to mark this PR dependent on that issue that you reopened for fixing the git guardian checks. That way if someone comes and looks at this they'll know exactly what happened and why is this PR stalling for now.

ruzell22 added a commit to ruzell22/cactus that referenced this pull request Apr 13, 2023
…#2379

fixes: hyperledger-cacti#2379

related to: hyperledger-cacti#2313 and hyperledger-cacti#1996

This fixes the Invalid token header, no credentials provided error
of the custom gitguardian workflow.

Signed-off-by: ruzell22 <[email protected]>
ruzell22 added a commit to ruzell22/cactus that referenced this pull request Apr 13, 2023
…#2379

fixes: hyperledger-cacti#2379

related to: hyperledger-cacti#2313 and hyperledger-cacti#1996

This fixes the Invalid token header, no credentials provided error
of the custom gitguardian workflow.

Signed-off-by: ruzell22 <[email protected]>
ruzell22 added a commit to ruzell22/cactus that referenced this pull request Apr 13, 2023
…#2379

fixes: hyperledger-cacti#2379

related to: hyperledger-cacti#2313 and hyperledger-cacti#1996

This fixes the Invalid token header, no credentials provided error
of the custom gitguardian workflow.

Signed-off-by: ruzell22 <[email protected]>
ruzell22 added a commit to ruzell22/cactus that referenced this pull request Apr 13, 2023
…#2379

fixes: hyperledger-cacti#2379

related to: hyperledger-cacti#2313 and hyperledger-cacti#1996

This fixes the Invalid token header, no credentials provided error
of the custom gitguardian workflow.

Signed-off-by: ruzell22 <[email protected]>
ruzell22 added a commit to ruzell22/cactus that referenced this pull request Apr 13, 2023
…#2379

fixes: hyperledger-cacti#2379

related to: hyperledger-cacti#2313 and hyperledger-cacti#1996

This fixes the Invalid token header, no credentials provided error
of the custom gitguardian workflow.

Signed-off-by: ruzell22 <[email protected]>
ruzell22 added a commit to ruzell22/cactus that referenced this pull request Apr 13, 2023
…#2379

fixes: hyperledger-cacti#2379

related to: hyperledger-cacti#2313 and hyperledger-cacti#1996

This fixes the Invalid token header, no credentials provided error
of the custom gitguardian workflow.

Signed-off-by: ruzell22 <[email protected]>
ruzell22 added a commit to ruzell22/cactus that referenced this pull request Apr 13, 2023
…#2379

fixes: hyperledger-cacti#2379

related to: hyperledger-cacti#2313 and hyperledger-cacti#1996

This fixes the Invalid token header, no credentials provided error
of the custom gitguardian workflow.

Signed-off-by: ruzell22 <[email protected]>
ruzell22 added a commit to ruzell22/cactus that referenced this pull request Apr 13, 2023
…#2379

fixes: hyperledger-cacti#2379

related to: hyperledger-cacti#2313 and hyperledger-cacti#1996

This fixes the Invalid token header, no credentials provided error
of the custom gitguardian workflow.

Signed-off-by: ruzell22 <[email protected]>
ruzell22 added a commit to ruzell22/cactus that referenced this pull request Apr 27, 2023
…#2379

fixes: hyperledger-cacti#2379

related to: hyperledger-cacti#2313 and hyperledger-cacti#1996

This fixes the Invalid token header, no credentials provided error
of the custom gitguardian workflow.

Signed-off-by: ruzell22 <[email protected]>
ruzell22 added a commit to ruzell22/cactus that referenced this pull request Apr 27, 2023
…#2379

fixes: hyperledger-cacti#2379

related to: hyperledger-cacti#2313 and hyperledger-cacti#1996

This fixes the Invalid token header, no credentials provided error
of the custom gitguardian workflow.

Signed-off-by: ruzell22 <[email protected]>
ruzell22 added a commit to ruzell22/cactus that referenced this pull request Apr 27, 2023
…#2379

fixes: hyperledger-cacti#2379

related to: hyperledger-cacti#2313 and hyperledger-cacti#1996

This fixes the Invalid token header, no credentials provided error
of the custom gitguardian workflow.

Signed-off-by: ruzell22 <[email protected]>
ruzell22 added a commit to ruzell22/cactus that referenced this pull request Apr 27, 2023
…#2379

fixes: hyperledger-cacti#2379

related to: hyperledger-cacti#2313 and hyperledger-cacti#1996

This fixes the Invalid token header, no credentials provided error
of the custom gitguardian workflow.

Signed-off-by: ruzell22 <[email protected]>
@petermetz petermetz self-assigned this Jul 24, 2023
@petermetz petermetz added this to the v2.0.0-alpha.2 milestone Jul 24, 2023
@ruzell22
Copy link
Contributor Author

Hello @petermetz , with gitguardian being removed, this PR is considered done and can be closed. Thank you.

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@ruzell22 Please

  1. rebase onto upstream/main
  2. Retest and see if this part of the commit message is still true: "Parameter keyPairPem cannot be removed as it results to an error in running the api server."
    2.1. Post the full error logs if it is still crashing or remove it otherwise.

@ruzell22
Copy link
Contributor Author

@petermetz I have rebased and ran 'yarn run configure' and it built successfully.
image

I tried removing the parameter keyPairPem and it is having error when building
image

@petermetz petermetz removed their assignment Sep 7, 2023
@petermetz petermetz force-pushed the cmdapiservercleanup720 branch 2 times, most recently from 1f00d24 to 7008aeb Compare September 8, 2023 08:30
@petermetz petermetz enabled auto-merge (rebase) September 8, 2023 08:30
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

Thank you @ruzzel22, LGTM

…er-cacti#720

BREAKING CHANGE: Removed the `keyPairPem` parameter from the API server
configuration.

fixes: hyperledger-cacti#720

Parameters cleaned up are: cactusNodeId, consortiumId, keychainSuffixKeyPairPem

Cleaning the three mentioned parameters are backwards compatible with tags
versions: v1.0.0-rc.3 and v1.0.0

The latest tag being used as of this change is v1.0.0-25-gdda3f00c

Signed-off-by: ruzell22 <[email protected]>
Signed-off-by: Peter Somogyvari <[email protected]>
@petermetz petermetz merged commit b8e8388 into hyperledger-cacti:main Sep 20, 2023
43 of 68 checks passed
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.

refactor(cmd-api-server): clean up configuration parameters
5 participants