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

new resources: realm key aes, ecdsa, hmac, rsa, java keystore #582

Merged
merged 31 commits into from
Oct 13, 2021
Merged

new resources: realm key aes, ecdsa, hmac, rsa, java keystore #582

merged 31 commits into from
Oct 13, 2021

Conversation

Vlad-Kirichenko
Copy link
Contributor

Hi @mrparkers,

This PR adds new resources to manage keystores. #569
Tried creating everything in one resource but decided to separate them.
That's why:

  • Easier data validation
  • Fewer fields need to be filled when creating a resource
  • The scheme for the config for the component is more complicated (also I faced the fact that if you pass empty fields to the keyloak, it will not update the resource, but it also does not throw an error)

Work is still in progress, here's what needs to be done:

  • Add tests for new resources
  • Fix resource import functions

Best regards,
Vlad

Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
@mrparkers
Copy link
Owner

Thanks for the PR @Vlad-Kirichenko, this is looking good so far. Let me know if you need help with anything.

Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
@Vlad-Kirichenko
Copy link
Contributor Author

Hello @mrparkers
I want to write a test for Java keystore. And for this I need to add an alias in the keystore inside instance with the keycloak.
Can you please advise the way to do this?

@mrparkers
Copy link
Owner

Sorry for the late response @Vlad-Kirichenko. Unfortunately, this is a feature of Keycloak that I'm not really familiar with, so I am not sure how to advise you on that test.

@Vlad-Kirichenko
Copy link
Contributor Author

That's all right. Then @mrparkers, what do you think if I will add step in .circleci/config.yml which will create java keystore and add alias into it(by using keytool) before Run Tests script?

@mrparkers
Copy link
Owner

Yeah I have no problem with that at all. If all this is doing is generating a file to mount to Keycloak's file system, you could also just generate it once and store it within provider/misc, which is where I put a cert / key for tests relating to SAML clients. Or we could make a dedicated folder for test artifacts.

Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
@ringods
Copy link

ringods commented Sep 17, 2021

Hello @mrparkers, joining the conversation here for a quick question. @Vlad-Kirichenko is one of my team members, and the work done here is contributed by the company we work for. As a technical lead, I was able to have an approved list of OpenSource projects we use internally, so we could contribute back to these projects. Executive management approved but they would like these contributions linked to their brand.

We would very much like to see a notion of this work logged somewhere, something like name of the company, hyperlinked to our website, and mentioning this PR and any later ones (e.g. #594).

Can you live with this? If so, where would be the best place to put this? The README.md file?

Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
@mrparkers
Copy link
Owner

I'm not opposed to something like that, but I'm not sure that the README is the appropriate place for this.

How about this - I'll make some changes to the changelog file, so that each release includes a "thank you" section that tags the individuals that contributed code towards that release. Once I've done that, you can submit a PR to update one of these entries to include something like "@Vlad-Kirichenko on behalf of Example Company". Does that sound like a good compromise?

@mrparkers
Copy link
Owner

Okay, I've gone ahead and updated the changelog to include a list of each contributor under every release. Once I've cut a release that includes these changes, feel free to make a PR to this file to include a reference to the company that these changes were contributed on behalf of.

@Vlad-Kirichenko Vlad-Kirichenko marked this pull request as ready for review September 20, 2021 06:37
@Vlad-Kirichenko
Copy link
Contributor Author

Hello @mrparkers.
This PR and #594 are ready to review.
Thank you!

Copy link
Owner

@mrparkers mrparkers left a comment

Choose a reason for hiding this comment

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

overall this looks really good, there's just a couple of issues I found

docs/resources/realm_keystore_aes_generated.md Outdated Show resolved Hide resolved
docs/resources/realm_keystore_aes_generated.md Outdated Show resolved Hide resolved
keycloak/realm_keystore_hmac_generated.go Outdated Show resolved Hide resolved
keycloak/realm_keystore_rsa.go Outdated Show resolved Hide resolved
provider/provider.go Outdated Show resolved Hide resolved
provider/resource_keycloak_realm_keystore_aes_generated.go Outdated Show resolved Hide resolved
func TestAccKeycloakRealmKeystoreJava_basic(t *testing.T) {
t.Parallel()

skipIfEnvSet(t, "CI") // temporary while I figure out how to put java keystore file to keycloak container in CI
Copy link
Owner

Choose a reason for hiding this comment

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

we could probably just mount the misc directory as a volume on the running container and reference it that way. but I'm fine with leaving this as-is for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I tried to to this(using the misc dir) but the problem that I need keystore file inside keycloak instance, not in runner instance.

In the terraform manifest inside keystore field I paste path for keycloak itself.

provider/resource_keycloak_realm_keystore_rsa.go Outdated Show resolved Hide resolved
provider/resource_keycloak_realm_keystore_rsa.go Outdated Show resolved Hide resolved
Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
Copy link
Owner

@mrparkers mrparkers left a comment

Choose a reason for hiding this comment

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

Just a few more comments, sorry for the delay. we can get this merged and released ASAP as soon as these are addressed.

docs/resources/realm_keystore_rsa.md Outdated Show resolved Hide resolved
docs/resources/realm_keystore_rsa.md Outdated Show resolved Hide resolved
provider/resource_keycloak_realm_keystore_java_keystore.go Outdated Show resolved Hide resolved
@StuxxNet
Copy link

Guys, do you have any news about this topic? I would love to use it here, because we have some configurations that were done manually, and now I need to import the keys.

@Vlad-Kirichenko
Copy link
Contributor Author

Hello @mrparkers. can you review this PR and #599?

Best regards,
Vlad

Copy link
Owner

@mrparkers mrparkers left a comment

Choose a reason for hiding this comment

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

Thanks for your patience, this looks great. Thanks again for your work on this! 🚀

@mrparkers mrparkers merged commit fb08f47 into mrparkers:master Oct 13, 2021
@Vlad-Kirichenko Vlad-Kirichenko deleted the realm-keys branch October 13, 2021 06:52
@StuxxNet
Copy link

@mrparkers when do you plan to release a new version with this resource available? :D

@ringods
Copy link

ringods commented Oct 13, 2021

@StuxxNet the company I work for is contributing 3 PRs, by way of my team member @Vlad-Kirichenko. Two of them are already merged, one is being finalised (#599). Are you OK that we get that last one in before a new release is created?

@StuxxNet
Copy link

@StuxxNet the company I work for is contributing 3 PRs, by way of my team member @Vlad-Kirichenko. Two of them are already merged, one is being finalised (#599). Are you OK that we get that last one in before a new release is created?

Totally ok with it, @ringods. Just asking so I can plan myself to start developing here :)

@ringods
Copy link

ringods commented Oct 14, 2021

@StuxxNet it seems @mrparkers merged our latest PR and created that release: v3.5.0 😄

@mrparkers
Copy link
Owner

Thanks again for your patience @ringods and @Vlad-Kirichenko. Feel free to open a PR to update this line of the changelog:

- [@Vlad-Kirichenko](https://github.com/Vlad-Kirichenko)
if you'd like to attribute these changes to the company you work for, as we discussed earlier.

@e42sh
Copy link
Contributor

e42sh commented Oct 18, 2021

Thanks @Vlad-Kirichenko

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.

5 participants