feat: Support to ory hydra running in secure mode #62

fjvierap · 2020-12-18T16:01:58Z

We have a hydra instance running in https and we cannot use hydra-maester to communicate with our hydra instance. Hence we created this extension which allows:

skip insecure verification in http client by adding insecure-skip-verify argument.
Use github.com/go-openapi/runtime/clienttls client when a certificate loaded as tls-trust-store argument.

CLAassistant · 2020-12-18T16:02:07Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ fjvierap
✅ colunira
❌ ORY Continuous Integration

ORY Continuous Integration seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

aeneasr · 2021-04-23T08:06:40Z

Closing due to inactivity

fjvierap · 2021-04-23T08:21:41Z

@aeneasr for curiosity why is this closed?
It was just pending to review I think... Or is there some question not resolved or the code does not look good and I missed?

aeneasr · 2021-04-23T08:34:06Z

sorry my bad

fjvierap · 2021-04-23T09:08:49Z

sorry my bad

No problem ;)

main.go

README.md

Co-authored-by: Jakub Błaszczyk <[email protected]>

Fail for tls trust store not presented

fjvierap · 2021-04-28T14:24:56Z

@Demonsthere I updated PR based on your comments.

Demonsthere · 2021-04-28T14:36:19Z

Hi there @fjvierap thanks for the update! I will check as soon as able :)

Demonsthere · 2021-04-29T08:19:59Z

It's looking great 👍 The only improvement I can think of is a small test maybe here to create a client with and without trustStore.

aeneasr · 2021-05-04T08:56:30Z

@Demonsthere should I wait for #62 (comment) ?

Demonsthere · 2021-05-04T09:04:06Z

TBH I'm conflicted here. After some evaluation I see that we do not have any test that verifies if the hydraClient is created correctly, which should be addressed. However, fixing that seems outside of the scope of this PR. Maybe we should accept this PR as is, and define a task with adding the missing tests?

aeneasr · 2021-05-04T09:50:47Z

According to our contribution guidelines, if it is required for you to verify that the change is doing what it is supposed to do, then asking for a test as a requirement for merging the PR is in scope :)

fjvierap · 2021-05-04T12:10:28Z

TBH I'm conflicted here. After some evaluation I see that we do not have any test that verifies if the hydraClient is created correctly, which should be addressed. However, fixing that seems outside of the scope of this PR. Maybe we should accept this PR as is, and define a task with adding the missing tests?

For that we will need to move getHydraClientMaker to a separate package... Which package name you suggest to move the function?

Demonsthere · 2021-05-04T16:05:52Z

imho, getHydraClientMaker is a wrapper for controllers.HydraClientMakerFunc, and we might even be able to get rid of it. Alternatively i'd say helpers package

fjvierap · 2021-05-05T09:55:32Z

imho, getHydraClientMaker is a wrapper for controllers.HydraClientMakerFunc, and we might even be able to get rid of it. Alternatively i'd say helpers package

Could you please check my last 2 commit and let me know if that is you expect?

Demonsthere

It's looking great! Just a few little improvements ;)

helpers/http_client_test.go

helpers/http_client.go

helpers/http_client_test.go

fjvierap · 2021-05-10T07:31:24Z

It's looking great! Just a few little improvements ;)

Hi I added the improvements you suggested ;)

Demonsthere · 2021-05-10T07:41:48Z

helpers/http_client_test.go

+	t.Run("should not create client with and wrong tlsTrustStore", func(t *testing.T) {
+		client, err := helpers.CreateHttpClient(true, "/somefile")
+		require.NotNil(t, client)
+		require.NotNil(t, err)


Since we control what error will be returned here, maybe we could check not only if the err is not nil, but if it the one we expect?

Demonsthere

I just noticed that the new package is not by default included in the makefile target so the tests for it won't run. Could you please add it there? Then we will see if the test are correct :)

Demonsthere

LGTM :)

aeneasr · 2021-05-10T08:35:42Z

Thank you! Could you please rebase/merge with master? Then we can merge it right away!

Co-authored-by: Jakub Błaszczyk <[email protected]>

Support to ory hydra running in secure mode

6491a99

feat: adjust readme

d769f7d

fjvierap changed the title ~~Support to ory hydra running in secure mode~~ feat: Support to ory hydra running in secure mode Dec 18, 2020

Add namespace bases roles instead of cluster

daf5c44

aeneasr closed this Apr 23, 2021

aeneasr reopened this Apr 23, 2021

aeneasr requested a review from Demonsthere April 23, 2021 08:34

aeneasr self-assigned this Apr 23, 2021

aeneasr added the feat New feature or request. label Apr 23, 2021

Add log message when enable insecure skip verify

7d66a70

Demonsthere requested changes Apr 23, 2021

View reviewed changes

main.go Outdated Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

fjvierap and others added 3 commits April 23, 2021 15:47

Add setup log for insecure verify

d90f5a9

Co-authored-by: Jakub Błaszczyk <[email protected]>

Adjust README.md

509ee6a

Co-authored-by: Jakub Błaszczyk <[email protected]>

Merge remote-tracking branch 'origin/master' into namespace

90f8234

aeneasr requested a review from Demonsthere April 27, 2021 16:35

fjvierap added 3 commits April 28, 2021 16:16

Add error for not existent tls trust store

715ee4a

Merge pull request #1 from fjvierap/namespace

51a4e30

Fail for tls trust store not presented

Adjust rbac

e16c267

Demonsthere approved these changes May 4, 2021

View reviewed changes

fjvierap added 2 commits May 5, 2021 11:25

Add unit test for create http client

b75d417

Remove HydraClientMaker

5364f44

Demonsthere requested changes May 6, 2021

View reviewed changes

Improve error handling

d403834

Demonsthere reviewed May 10, 2021

View reviewed changes

Demonsthere requested changes May 10, 2021

View reviewed changes

Add helpers to makefile for testing that package

d12ad0a

Demonsthere approved these changes May 10, 2021

View reviewed changes

build: update CRDs and k8s dependencies (ory#68)

9b8f463

ORY Continuous Integration and others added 14 commits May 10, 2021 08:50

docs: Incorporates changes from version v0.0.20

9d56503

Support to ory hydra running in secure mode

14611a8

feat: adjust readme

7f2bf13

Add namespace bases roles instead of cluster

cde714e

Add log message when enable insecure skip verify

bbd2830

Add setup log for insecure verify

a210259

Co-authored-by: Jakub Błaszczyk <[email protected]>

Adjust README.md

00d3a80

Co-authored-by: Jakub Błaszczyk <[email protected]>

Add error for not existent tls trust store

1ff476c

Adjust rbac

b8622e0

Add unit test for create http client

ba61fce

Remove HydraClientMaker

9c5a153

Improve error handling

27558c3

Add helpers to makefile for testing that package

aaf8870

Merge branch 'master' of https://github.com/fjvierap/hydra-maester

80230cd

aeneasr merged commit 0ac5779 into ory:master May 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support to ory hydra running in secure mode #62

feat: Support to ory hydra running in secure mode #62

fjvierap commented Dec 18, 2020

CLAassistant commented Dec 18, 2020 •

edited

Loading

aeneasr commented Apr 23, 2021

fjvierap commented Apr 23, 2021 •

edited

Loading

aeneasr commented Apr 23, 2021

fjvierap commented Apr 23, 2021

fjvierap commented Apr 28, 2021

Demonsthere commented Apr 28, 2021

Demonsthere commented Apr 29, 2021

aeneasr commented May 4, 2021

Demonsthere commented May 4, 2021

aeneasr commented May 4, 2021

fjvierap commented May 4, 2021

Demonsthere commented May 4, 2021

fjvierap commented May 5, 2021

Demonsthere left a comment

fjvierap commented May 10, 2021

Demonsthere May 10, 2021

Demonsthere left a comment

Demonsthere left a comment

aeneasr commented May 10, 2021

feat: Support to ory hydra running in secure mode #62

feat: Support to ory hydra running in secure mode #62

Conversation

fjvierap commented Dec 18, 2020

CLAassistant commented Dec 18, 2020 • edited Loading

aeneasr commented Apr 23, 2021

fjvierap commented Apr 23, 2021 • edited Loading

aeneasr commented Apr 23, 2021

fjvierap commented Apr 23, 2021

fjvierap commented Apr 28, 2021

Demonsthere commented Apr 28, 2021

Demonsthere commented Apr 29, 2021

aeneasr commented May 4, 2021

Demonsthere commented May 4, 2021

aeneasr commented May 4, 2021

fjvierap commented May 4, 2021

Demonsthere commented May 4, 2021

fjvierap commented May 5, 2021

Demonsthere left a comment

Choose a reason for hiding this comment

fjvierap commented May 10, 2021

Demonsthere May 10, 2021

Choose a reason for hiding this comment

Demonsthere left a comment

Choose a reason for hiding this comment

Demonsthere left a comment

Choose a reason for hiding this comment

aeneasr commented May 10, 2021

CLAassistant commented Dec 18, 2020 •

edited

Loading

fjvierap commented Apr 23, 2021 •

edited

Loading