fix(credentials): set backend storage as default #672

tthvo · 2022-11-18T03:46:47Z

Related to #656

Use Backend storage as default. This does not fix the issue but just switch it default Backend.

src/app/AppLayout/AuthModal.tsx

tthvo · 2022-11-18T06:25:10Z

Actually, I messed around with UI and somehow found this way to consistently reproduce it. The steps are similar to the description of #673

To reproduce

Run backend with:

$ CRYOSTAT_DISABLE_SSL=true CRYOSTAT_CORS_ORIGIN=http://localhost:9000 sh smoketest.sh

Run front-end with:
```
$ yarn start:dev
```
Login and go to settings to set storage options to Browser Session.
Then, go back and select target io.cryostat.Cryostat.
Enter http://localhost:9000/recordings/create on browser.
Dismiss auth form modal.
Choose snapshot recording tab.
Click create.
Enter credentials (smoketest).
See error occurs.

Could you try if this happens on your end too?

Observations

Requests are sent with correct username/password (i.e. credential map holds correct record and console.log shows correct Basic header being added).

andrewazores · 2022-11-18T14:29:28Z

Yes, these reproduction steps work for me.

Interestingly, the Authorization header and the X-JMX-Authorization header are actually being sent with the first request according to the browser devtools network tab, and yet the response is a 427 failure. But if I navigate over to the Events view, for example, and enter the JMX credentials on the modal, then I can see requests going out with the same headers and yet receiving 200 responses.

andrewazores · 2022-11-18T14:33:10Z

Recordings (broken):

Request:

GET /api/v1/targets/service%3Ajmx%3Armi%3A%2F%2F%2Fjndi%2Frmi%3A%2F%2Fcryostat%3A9091%2Fjmxrmi/recordings HTTP/1.1
Host: localhost:8181
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:106.0) Gecko/20100101 Firefox/106.0
Accept: */*
Accept-Language: en-CA,en-US;q=0.7,en;q=0.3
Accept-Encoding: gzip, deflate, br
Referer: http://localhost:9000/
authorization: Basic dXNlcjpwYXNz
x-jmx-authorization: Basic c21va2V0ZXN0OnNtb2tldGVzdA==
Origin: http://localhost:9000
DNT: 1
Connection: keep-alive
Sec-Fetch-Dest: empty
Sec-Fetch-Mode: cors
Sec-Fetch-Site: same-site

Response:

HTTP/1.1 427 Client Error (427)
vary: origin
access-control-allow-credentials: true
access-control-allow-origin: http://localhost:9000
access-control-expose-headers: X-WWW-Authenticate,X-JMX-Authenticate
X-JMX-Authenticate: Basic
content-type: text/plain
content-encoding: gzip
content-length: 104

Events (working):

Request:

GET /api/v1/targets/service%3Ajmx%3Armi%3A%2F%2F%2Fjndi%2Frmi%3A%2F%2Fcryostat%3A9091%2Fjmxrmi/events HTTP/1.1
Host: localhost:8181
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:106.0) Gecko/20100101 Firefox/106.0
Accept: */*
Accept-Language: en-CA,en-US;q=0.7,en;q=0.3
Accept-Encoding: gzip, deflate, br
Referer: http://localhost:9000/
authorization: Basic dXNlcjpwYXNz
x-jmx-authorization: Basic c21va2V0ZXN0OnNtb2tldGVzdA==
Origin: http://localhost:9000
DNT: 1
Connection: keep-alive
Sec-Fetch-Dest: empty
Sec-Fetch-Mode: cors
Sec-Fetch-Site: same-site

Response:

HTTP/1.1 200 OK
vary: origin
access-control-allow-credentials: true
access-control-allow-origin: http://localhost:9000
access-control-expose-headers: X-WWW-Authenticate,X-JMX-Authenticate
content-type: application/json
content-encoding: gzip
content-length: 4793

andrewazores · 2022-11-18T14:34:08Z

So it looks like this is probably actually a backend bug after all. Setting the credentials storage to Backend is still an effective workaround.

andrewazores · 2022-11-18T14:37:43Z

Also very interesting. After reproducing the bug with Thuan's steps above, I tried setting the storage to Backend, then closing and reopening the web-client browser tab. Then I went back to the Recordings view. There were multiple snapshot-n recordings on the target that were just created in the minutes prior. So somehow the frontend was actually sending requests to create snapshots even though I had only navigate to the snapshot creation view but not clicked the creation button, and yet the responses to those requests were 427 statuses even though it appears the request succeeded.

tthvo · 2022-11-18T18:31:05Z

Also very interesting. After reproducing the bug with Thuan's steps above, I tried setting the storage to Backend, then closing and reopening the web-client browser tab. Then I went back to the Recordings view. There were multiple snapshot-n recordings on the target that were just created in the minutes prior. So somehow the frontend was actually sending requests to create snapshots even though I had only navigate to the snapshot creation view but not clicked the creation button, and yet the responses to those requests were 427 statuses even though it appears the request succeeded.

Interesting, i saw the same issue. It seems like its only broken in Recording view. Should we transfer this issue to the backend repo? Also are we good with this workaround?

andrewazores · 2022-11-18T18:37:49Z

Let's confirm the exact root cause of the problem before transferring the issue, just so we don't end up transferring it back and forth. Worth considering still that there are backend itests that exercise what should be the same code path and those pass, so there's something odd going on at some level or another and I don't think we've fully nailed down what or where it is yet.

Anyway I'm good with the workaround of setting the default to backend. That's actually closer to what 2.1 was doing anyway.

Could you split out the change that changes the default into a separate PR, which can be merged and backported separately without these other refactoring fixes? Only the change of default should go into a backport for 2.2.1.

tthvo · 2022-11-18T18:49:58Z

Could you split out the change that changes the default into a separate PR, which can be merged and backported separately without these other refactoring fixes? Only the change of default should go into a backport for 2.2.1.

Okayy updated :D I will open another PR for the clean up.

maxcao13 · 2022-11-18T18:54:57Z

Just confirming that I see the same issue.

My first instinct is that it might be because computing a jvmId uses back-end credentials instead of using session memory? I'm not sure if it does, but I see after performing the snapshot request:

org.openjdk.jmc.rjmx.ConnectionException caused by java.lang.SecurityException: Authentication failed! Invalid username or password
	at org.openjdk.jmc.rjmx.internal.RJMXConnection.connect(RJMXConnection.java:441)
	at io.cryostat.core.net.JFRConnection.attemptConnect(JFRConnection.java:310)
	at io.cryostat.core.net.JFRConnection.connect(JFRConnection.java:267)
	at io.cryostat.core.net.JFRConnection.getJvmId(JFRConnection.java:178)
	at io.cryostat.recordings.JvmIdHelper.lambda$computeJvmId$4(JvmIdHelper.java:161)
	at io.cryostat.net.TargetConnectionManager.lambda$executeConnectedTaskAsync$2(TargetConnectionManager.java:148)
	at java.base/java.util.concurrent.CompletableFuture$UniApply.tryFire(CompletableFuture.java:646)
	at java.base/java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:483)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:373)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1182)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1655)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1622)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:165)
Caused by: java.lang.SecurityException: Authentication failed! Invalid username or password

then maybe it tries to use credentials it can't find...

andrewazores · 2022-11-18T18:57:11Z

Aha, that's a good insight. I didn't check the backend logs during the test run but that makes a lot of sense. I think that's the next most obvious candidate bug to work through. If that is the cause then in order for the Session storage to work properly again we'll need to introduce some larger notion of a request state/session on the backend so that the JMX credentials can follow through the execution path. That might be tough. Though, I'm not sure then why this bug manifests on the Recordings view and not Events.

(cherry picked from commit 7ba910e) # Conflicts: # src/test/Settings/CredentialsStorage.test.tsx

maxcao13 · 2022-11-18T19:09:36Z

Aha, that's a good insight. I didn't check the backend logs during the test run but that makes a lot of sense. I think that's the next most obvious candidate bug to work through. If that is the cause then in order for the Session storage to work properly again we'll need to introduce some larger notion of a request state/session on the backend so that the JMX credentials can follow through the execution path. That might be tough. Though, I'm not sure then why this bug manifests on the Recordings view and not Events.

I think it's just cause eventGet endpoint doesn't generate or get jvmIds but things like RecordingPost, SnapshotPost, MetadataPost all do... I think everything glaringly points to jvmIds at this point, and I agree there the two methods of using credentials should somehow follow the same execution path to compute the ids for the session state to work. I'll see if I can try something to fix this.

andrewazores · 2022-11-18T19:15:21Z

Ahh, those things need the JVM ID present because they do things like storing metadata or archives, and nothing in the Events view does that so the JVM ID code path never gets hit. I see.

Yea... that's a little rough. So for session authentication to continue working we do need some way to allow those credentials to pass from the API handlers all the way through the call stack. Not conceptually hard but that's going to be a large refactoring surface.

maxcao13 · 2022-11-18T19:17:26Z

Yes, stuff like

this.targetConnectionManager.executeConnectedTaskAsync(
                            new ConnectionDescriptor(
                                    sr.getServiceUri().toString(),
                                    credentialsManager.getCredentials(sr)),
                            connection -> {
                                return connection.getJvmId();

wouldn't work because the credentialsManager is only searching the backend for credentials and is unaware of any session state or headers.

andrewazores · 2022-11-18T19:18:10Z

So then perhaps we don't actually have any integration tests that exercise this path then, right? I know we have tests that exercise starting recordings and taking snapshots, test that exercise retrieving event types and templates, and tests that use JMX Auth, but maybe we don't have tests that try to start recordings on targets that need auth.

$ ag X-JMX-Authorization src/test/java/itest/
src/test/java/itest/JmxAuthIT.java
60:    private static final String X_JMX_AUTHORIZATION = "X-JMX-Authorization";

src/test/java/itest/util/http/JvmIdWebRequest.java
86:                    "X-JMX-Authorization",

src/test/java/itest/AutoRulesIT.java
291:                        "X-JMX-Authorization",
376:                            "X-JMX-Authorization",
475:                            "X-JMX-Authorization",
599:                            "X-JMX-Authorization",
638:                            "X-JMX-Authorization",

src/test/java/itest/InterleavedExternalTargetRequestsIT.java
211:                                        "X-JMX-Authorization",
241:                            "X-JMX-Authorization",
285:                                        "X-JMX-Authorization",

andrewazores · 2022-11-18T19:20:04Z

RecordingWorkflowIT would be the obvious candidate for an integration test to update and add X-JMX-Authorization credentials to for simulating the frontend session setting, assuming none of those tests above are already doing something similar.

Though, it looks like InterleavedExternalTargetRequestsIT is already doing that:

    private void createInMemoryRecordings() throws Exception {
        List<CompletableFuture<Void>> cfs = new ArrayList<>();
        for (int i = 0; i < CONTAINERS.size(); i++) {
            final int fi = i;
            CompletableFuture<Void> cf = new CompletableFuture<>();
            cfs.add(cf);
            Podman.POOL.submit(
                    () -> {
                        MultiMap form = MultiMap.caseInsensitiveMultiMap();
                        form.add("recordingName", "interleaved-" + fi);
                        form.add("events", "template=Continuous");
                        webClient
                                .post(
                                        String.format(
                                                "/api/v1/targets/%s/recordings",
                                                Podman.POD_NAME + ":" + (9093 + fi)))
                                .putHeader(
                                        "X-JMX-Authorization",
                                        "Basic "
                                                + Base64.getUrlEncoder()
                                                        .encodeToString(
                                                                "admin:adminpass123".getBytes()))
                                .sendForm(
                                        form,
                                        ar -> {
                                            if (assertRequestStatus(ar, cf)) {
                                                cf.complete(null);
                                            }
                                        });
                    });
        }
        CompletableFuture.allOf(cfs.toArray(new CompletableFuture[0]))
                .get(REQUEST_TIMEOUT_SECONDS, TimeUnit.SECONDS);
    }

maxcao13 · 2022-11-18T19:22:54Z

Though, it looks like InterleavedExternalTargetRequestsIT is already doing that:
...code

Is JMX auth enabled for those targets? Don't they need a key value pair of "JMX_AUTH, true" in the new Podman.ImageSpec envs or something like that?

andrewazores · 2022-11-18T19:25:05Z

This might be something we wait to fix until Cryostat 3.0, honestly. It seems to me the best way to handle this is to rearchitect a lot of Cryostat's internal infastructure to be scope to the lifetime of the request rather than having a static lifetime for the whole application. ie, there would be a new TargetConnectionManager instance created for each request that needs one, with direct access to the request context for retrieving X-JMX-Authorization header credentials, and a new ActiveRecordingHelper also scoped to the request lifecycle for the same reason, etc.

andrewazores · 2022-11-18T19:26:03Z

Though, it looks like InterleavedExternalTargetRequestsIT is already doing that:
...code

Is JMX auth enabled for those targets? Don't they need a key value pair of "JMX_AUTH, true" in the new Podman.ImageSpec envs or something like that?

Ha. You are correct, the test is broken and the external app containers don't actually require JMX auth. Wow...

andrewazores · 2022-11-18T19:27:04Z

    @BeforeAll
    static void setup() throws Exception {
        Set<Podman.ImageSpec> specs = new HashSet<>();
        for (int i = 0; i < NUM_EXT_CONTAINERS; i++) {
            specs.add(
                    new Podman.ImageSpec(
                            FIB_DEMO_IMAGESPEC, Map.of("JMX_PORT", String.valueOf(9093 + i))));
             -->            // This Map also needs "USE_AUTH", "true"
        }
        for (Podman.ImageSpec spec : specs) {
            CONTAINERS.add(Podman.run(spec));
        }
        CompletableFuture.allOf(
                        CONTAINERS.stream()
                                .map(id -> Podman.waitForContainerState(id, "running"))
                                .collect(Collectors.toList())
                                .toArray(new CompletableFuture[0]))
                .join();
        waitForDiscovery(NUM_EXT_CONTAINERS);
    }

* fix(credentials): set backend storage as default (#672) (cherry picked from commit 7ba910e) # Conflicts: # src/test/Settings/CredentialsStorage.test.tsx * resolve conflict Co-authored-by: Thuan Vo <thvo@redhat.com>

andrewazores · 2022-11-29T16:10:40Z

@maxcao13 @tthvo would one of you like to take on fixing that itest above so it actually sets up the containers to require credentials? It might end up failing right now, so I think we can mark it as a known failure like this: https://stackoverflow.com/questions/4055022/mark-unit-test-as-an-expected-failure-in-junit , at least until we come up with some fix for the implementation.

maxcao13 · 2022-11-29T16:14:05Z

would one of you like to take on fixing that itest above so it actually sets up the containers to require credentials? It might end up failing right now, so I think we can mark it as a known failure like this: https://stackoverflow.com/questions/4055022/mark-unit-test-as-an-expected-failure-in-junit , at least until we come up with some fix for the implementation.

Sure I can do that.

tthvo added chore Refactor, rename, cleanup, etc. fix labels Nov 18, 2022

tthvo requested review from andrewazores and maxcao13 November 18, 2022 03:46

tthvo commented Nov 18, 2022

View reviewed changes

src/app/AppLayout/AuthModal.tsx Outdated Show resolved Hide resolved

tthvo force-pushed the jmx-default-mode branch from 4c9d156 to b07327b Compare November 18, 2022 05:30

fix(credentials): set backend storage as default

a57178f

tthvo force-pushed the jmx-default-mode branch from b07327b to a57178f Compare November 18, 2022 18:46

tthvo changed the title ~~fix(credentials): set backend storage as default and refactor auth form~~ fix(credentials): set backend storage as default Nov 18, 2022

andrewazores approved these changes Nov 18, 2022

View reviewed changes

andrewazores added the backport label Nov 18, 2022

andrewazores merged commit 7ba910e into cryostatio:main Nov 18, 2022

mergify bot pushed a commit that referenced this pull request Nov 18, 2022

fix(credentials): set backend storage as default (#672)

73686b8

(cherry picked from commit 7ba910e) # Conflicts: # src/test/Settings/CredentialsStorage.test.tsx

mergify bot mentioned this pull request Nov 18, 2022

fix(credentials): set backend storage as default (backport #672) #674

Merged

andrewazores removed the chore Refactor, rename, cleanup, etc. label Nov 18, 2022

tthvo deleted the jmx-default-mode branch November 18, 2022 19:12

maxcao13 mentioned this pull request Nov 22, 2022

[Task] "Session credentials" mode seems unreliable for JMX connections #656

Closed

maxcao13 mentioned this pull request Nov 29, 2022

[Task] Fix itest for containers that need credentials cryostatio/cryostat-legacy#1262

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(credentials): set backend storage as default #672

fix(credentials): set backend storage as default #672

tthvo commented Nov 18, 2022 •

edited

Loading

tthvo commented Nov 18, 2022 •

edited

Loading

andrewazores commented Nov 18, 2022

andrewazores commented Nov 18, 2022

andrewazores commented Nov 18, 2022

andrewazores commented Nov 18, 2022

tthvo commented Nov 18, 2022

andrewazores commented Nov 18, 2022

tthvo commented Nov 18, 2022

maxcao13 commented Nov 18, 2022 •

edited

Loading

andrewazores commented Nov 18, 2022 •

edited

Loading

maxcao13 commented Nov 18, 2022 •

edited

Loading

andrewazores commented Nov 18, 2022

maxcao13 commented Nov 18, 2022 •

edited

Loading

andrewazores commented Nov 18, 2022

andrewazores commented Nov 18, 2022 •

edited

Loading

maxcao13 commented Nov 18, 2022 •

edited

Loading

andrewazores commented Nov 18, 2022

andrewazores commented Nov 18, 2022

andrewazores commented Nov 18, 2022 •

edited

Loading

andrewazores commented Nov 29, 2022

maxcao13 commented Nov 29, 2022

fix(credentials): set backend storage as default #672

fix(credentials): set backend storage as default #672

Conversation

tthvo commented Nov 18, 2022 • edited Loading

tthvo commented Nov 18, 2022 • edited Loading

To reproduce

Observations

andrewazores commented Nov 18, 2022

andrewazores commented Nov 18, 2022

andrewazores commented Nov 18, 2022

andrewazores commented Nov 18, 2022

tthvo commented Nov 18, 2022

andrewazores commented Nov 18, 2022

tthvo commented Nov 18, 2022

maxcao13 commented Nov 18, 2022 • edited Loading

andrewazores commented Nov 18, 2022 • edited Loading

maxcao13 commented Nov 18, 2022 • edited Loading

andrewazores commented Nov 18, 2022

maxcao13 commented Nov 18, 2022 • edited Loading

andrewazores commented Nov 18, 2022

andrewazores commented Nov 18, 2022 • edited Loading

maxcao13 commented Nov 18, 2022 • edited Loading

andrewazores commented Nov 18, 2022

andrewazores commented Nov 18, 2022

andrewazores commented Nov 18, 2022 • edited Loading

andrewazores commented Nov 29, 2022

maxcao13 commented Nov 29, 2022

tthvo commented Nov 18, 2022 •

edited

Loading

tthvo commented Nov 18, 2022 •

edited

Loading

maxcao13 commented Nov 18, 2022 •

edited

Loading

andrewazores commented Nov 18, 2022 •

edited

Loading

maxcao13 commented Nov 18, 2022 •

edited

Loading

maxcao13 commented Nov 18, 2022 •

edited

Loading

andrewazores commented Nov 18, 2022 •

edited

Loading

maxcao13 commented Nov 18, 2022 •

edited

Loading

andrewazores commented Nov 18, 2022 •

edited

Loading