Add option to authenticate via client certificates updated #166

flobz · 2023-09-27T12:00:29Z

Hello,
I updated this PR: #38 to apply @Bastian-Krause recommendation.
If there anything else to before merging ?

Bastian-Krause

It would be great to have an actual test case for this with a reverse proxy. We already use nginx for some test cases (see test/conftest.py).

This would be really useful.

Bastian-Krause · 2023-10-04T10:30:47Z

docs/using.rst

+ssl_verify                = true
+tenant_id                 = DEFAULT
+target_name               = test-target
+auth_token                =


Shouldn't this be commented, too?

Bastian-Krause · 2023-10-04T10:37:17Z

src/config-file.c

+        key_client_cert_exists = get_key_string(ini_file, "client", "client_cert", &config->client_cert, NULL, NULL);
+
+        key_client_key_exists = get_key_string(ini_file, "client", "client_key", &config->client_key, NULL, NULL);
+
+        if (!key_auth_token_exists && !key_gateway_token_exists &&  !(key_client_cert_exists && key_client_key_exists)) {
+                g_set_error(error, 1, 4, "Neither a token nor client certificate are set!");
                return NULL;
        }
-        if (key_auth_token_exists && key_gateway_token_exists) {
+        else if (key_auth_token_exists && key_gateway_token_exists) {


As far as I can see, this allows various strange combinations of auth/gateway token and cert/key:

auth_token + client_cert + client_key

gateway_token + client_cert + client_key

Since there a no later checks, both auth methods would need to be present. Does hawkBit support these cases?

auth_token + client_cert

auth_token + client_key

gateway_token + client_cert

gateway_token + client_key

These cases lead to auth_token or gateway_token being used, but no warning/error. That should be fixed.

It's possible to have two level of security in the same time:

Reverse proxy do authentication with cert

Hawkbit with token

So in my opinion we should allow token+ cert.

Okay, this should be documented then. The last 4 scenarios should be fixed.

Is this enough documented : https://github.com/rauc/rauc-hawkbit-updater/pull/166/files#diff-00d05b0405040981002c129a067f3ae336e345b5b62512322b116f63aec6c341R62 ?
What do you mean by 'The last 4 scenarios should be fixed." ?

Bastian-Krause · 2023-10-04T10:47:07Z

docs/using.rst

+ssl                       = true
+ssl_verify                = true


From #38 (comment):

What happens if these options are false?

I added the answer here : https://github.com/flobz/rauc-hawkbit-updater/blob/419925f13ac05d311d92e9129661783456726d78/docs/using.rst?plain=1#L66
If the CA of the reverse proxy server is untrusted set ssl_verify to false.

Okay, but what about the ssl option? That is required, right? Should be documented and a run time check would also be nice.

you're right I forgot this check : c972261

Bastian-Krause · 2023-10-04T10:52:32Z

src/hawkbit-client.c

+                curl_easy_setopt(curl, CURLOPT_SSLCERT, hawkbit_config->client_cert);
+                curl_easy_setopt(curl, CURLOPT_SSLKEY, hawkbit_config->client_key);


The existence of these files was never checked. What happens if they don't exist?

I add a check 118b83d

Bastian-Krause · 2023-10-04T10:54:57Z

docs/using.rst

-Authentication
--------------


Please leave this section as is and add the sections below as subsections.

Client certificate and privat key of Client certificate are send to the set Server. Add description on how to use this authentication method in "docs/using.rst" file. Signed-off-by: Cem Tenruh <[email protected]>

Signed-off-by: Cem Tenruh <[email protected]>

Signed-off-by: Florian Bezannier <[email protected]>

flobz · 2023-10-06T15:08:17Z

In #38 (comment), I said:

It would be great to have an actual test case for this with a reverse proxy. We already use nginx for some test cases (see test/conftest.py).

This would be really useful.

I add test but there is an issue. Hawkbit doesn't support downloading artifacts using http and https in the same time.
I add a PR on hawkbit repo eclipse-hawkbit/hawkbit#1449 but for now two hawkbit container config are needed.

flobz · 2023-10-18T05:35:56Z

@Bastian-Krause can you restart the "workflow" please ? I would like to check if change made to the pipeline works.

ejoerns · 2023-10-18T05:41:21Z

I've just triggered the CI workflow

Signed-off-by: Florian Bezannier <[email protected]>

flobz · 2023-10-24T07:26:07Z

Ok I fixed the CI but I don't understand why it worked before and why it doesn't work anymore...
I had to add this b2d77d9.
Can you restart the workflow please ?

Bastian-Krause · 2023-10-24T09:56:40Z

This will probably need to wait a few more weeks until I have time to review this.

…s in the same time Signed-off-by: Florian Bezannier <[email protected]>

flobz · 2023-12-05T16:55:37Z

I add test but there is an issue. Hawkbit doesn't support downloading artifacts using http and https in the same time. I add a PR on hawkbit repo eclipse/hawkbit#1449 but for now two hawkbit container config are needed.

It's fixed in hawkbit 0.3.0.

@Bastian-Krause would you be able to do a review soon ?

flobz force-pushed the WIP/ctenruh-phytec/client_certificates branch 4 times, most recently from 726d39e to 04676d2 Compare September 27, 2023 12:55

Bastian-Krause mentioned this pull request Oct 4, 2023

Add option to authenticate via client certificates #38

Closed

Bastian-Krause requested changes Oct 4, 2023

View reviewed changes

ctenruh-phytec added 2 commits October 6, 2023 16:07

Add option to authenticate via client certificates

3dad68e

Client certificate and privat key of Client certificate are send to the set Server. Add description on how to use this authentication method in "docs/using.rst" file. Signed-off-by: Cem Tenruh <[email protected]>

test/rauc-hawkbit-updater: New text in "expected output"

4654514

Signed-off-by: Cem Tenruh <[email protected]>

flobz force-pushed the WIP/ctenruh-phytec/client_certificates branch from 04676d2 to 419925f Compare October 6, 2023 14:53

flobz added 3 commits October 6, 2023 17:03

update doc

f7eabf9

Signed-off-by: Florian Bezannier <[email protected]>

mv curl cert client option to set_default_curl_opts

01c24a7

Signed-off-by: Florian Bezannier <[email protected]>

simplify auth method check

d8e6106

Signed-off-by: Florian Bezannier <[email protected]>

flobz force-pushed the WIP/ctenruh-phytec/client_certificates branch from 419925f to 9a2bb21 Compare October 6, 2023 15:04

flobz added 5 commits October 6, 2023 17:05

docs: update mtls doc

ba7bf4c

Signed-off-by: Florian Bezannier <[email protected]>

mtls: check if cert and key exist

c7e2506

Signed-off-by: Florian Bezannier <[email protected]>

mtls: add http header even if the is no auth token

1e32697

Signed-off-by: Florian Bezannier <[email protected]>

mtls: handle auth without token

c667f50

Signed-off-by: Florian Bezannier <[email protected]>

hawkbit-client: add url and method value in debug mode

1a42d68

Signed-off-by: Florian Bezannier <[email protected]>

flobz force-pushed the WIP/ctenruh-phytec/client_certificates branch from 9a2bb21 to 28cf12b Compare October 6, 2023 15:05

Bastian-Krause added the enhancement New feature or request label Oct 9, 2023

flobz force-pushed the WIP/ctenruh-phytec/client_certificates branch 3 times, most recently from 6d90f92 to 146096a Compare October 10, 2023 19:02

flobz added 4 commits October 24, 2023 09:09

mtls: add test

4082d02

Signed-off-by: Florian Bezannier <[email protected]>

ssl config option must be true if certificate authentication

8ca49cc

Signed-off-by: Florian Bezannier <[email protected]>

add mtls test to ci

8d876f7

Signed-off-by: Florian Bezannier <[email protected]>

docs: add dependancies

5f1d3e0

Signed-off-by: Florian Bezannier <[email protected]>

flobz added 2 commits October 24, 2023 09:13

fix: slow download test

b2d77d9

Signed-off-by: Florian Bezannier <[email protected]>

tests: add compatibility with libcurl>=8.0.0

4816283

Signed-off-by: Florian Bezannier <[email protected]>

flobz force-pushed the WIP/ctenruh-phytec/client_certificates branch from 146096a to 4816283 Compare October 24, 2023 07:21

flobz requested a review from Bastian-Krause October 24, 2023 10:11

revert: serparate mtls test because hawkbit 0.3.0 allow http and http…

99dc571

…s in the same time Signed-off-by: Florian Bezannier <[email protected]>

flobz force-pushed the WIP/ctenruh-phytec/client_certificates branch from 0770400 to 99dc571 Compare December 5, 2023 15:50

Bastian-Krause mentioned this pull request Feb 15, 2024

Add support for SSL/TLS client authentication #169

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to authenticate via client certificates updated #166

Add option to authenticate via client certificates updated #166

flobz commented Sep 27, 2023 •

edited

Loading

Bastian-Krause left a comment

Bastian-Krause Oct 4, 2023

flobz Oct 6, 2023

Bastian-Krause Oct 4, 2023

flobz Oct 4, 2023

Bastian-Krause Oct 9, 2023

flobz Oct 10, 2023

Bastian-Krause Oct 4, 2023

flobz Oct 6, 2023

Bastian-Krause Oct 9, 2023

flobz Oct 10, 2023 •

edited

Loading

Bastian-Krause Oct 4, 2023

flobz Oct 6, 2023

Bastian-Krause Oct 4, 2023

flobz Oct 6, 2023

flobz commented Oct 6, 2023 •

edited

Loading

flobz commented Oct 18, 2023

ejoerns commented Oct 18, 2023

flobz commented Oct 24, 2023

Bastian-Krause commented Oct 24, 2023

flobz commented Dec 5, 2023 •

edited

Loading

		curl_easy_setopt(curl, CURLOPT_SSLCERT, hawkbit_config->client_cert);
		curl_easy_setopt(curl, CURLOPT_SSLKEY, hawkbit_config->client_key);

		ssl = true
		ssl_verify = true

		Authentication
		--------------

Add option to authenticate via client certificates updated #166

Are you sure you want to change the base?

Add option to authenticate via client certificates updated #166

Conversation

flobz commented Sep 27, 2023 • edited Loading

Bastian-Krause left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flobz Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flobz commented Oct 6, 2023 • edited Loading

flobz commented Oct 18, 2023

ejoerns commented Oct 18, 2023

flobz commented Oct 24, 2023

Bastian-Krause commented Oct 24, 2023

flobz commented Dec 5, 2023 • edited Loading

flobz commented Sep 27, 2023 •

edited

Loading

flobz Oct 10, 2023 •

edited

Loading

flobz commented Oct 6, 2023 •

edited

Loading

flobz commented Dec 5, 2023 •

edited

Loading