-
Notifications
You must be signed in to change notification settings - Fork 303
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
Feature/ssh/show ssh fingerprints 2 #9958
Changes from all commits
5ed5b67
ac24f79
6ec63ce
785a691
41aae09
5115383
d35c826
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,50 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
package de.tum.cit.aet.artemis.programming.service.localvc.ssh; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import static de.tum.cit.aet.artemis.core.config.Constants.PROFILE_LOCALVC; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import java.io.IOException; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import java.security.GeneralSecurityException; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import java.util.HashMap; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import java.util.Map; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import jakarta.ws.rs.BadRequestException; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import org.apache.sshd.common.keyprovider.KeyPairProvider; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import org.apache.sshd.server.SshServer; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import org.slf4j.Logger; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import org.slf4j.LoggerFactory; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import org.springframework.context.annotation.Profile; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import org.springframework.stereotype.Service; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* Service responsible for providing SSH fingerprints of the SSH server running in Artemis. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
@Profile(PROFILE_LOCALVC) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
@Service | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public class SshFingerprintsProviderService { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
private static final Logger log = LoggerFactory.getLogger(SshFingerprintsProviderService.class); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
private final SshServer sshServer; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public SshFingerprintsProviderService(SshServer sshServer) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.sshServer = sshServer; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public Map<String, String> getSshFingerPrints() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Map<String, String> fingerprints = new HashMap<>(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
KeyPairProvider keyPairProvider = sshServer.getKeyPairProvider(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (keyPairProvider != null) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
keyPairProvider.loadKeys(null).iterator() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.forEachRemaining(keyPair -> fingerprints.put(keyPair.getPublic().getAlgorithm(), HashUtils.getSha256Fingerprint(keyPair.getPublic()))); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+34
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add null checks and validate fingerprint format The current implementation lacks proper null checks and fingerprint format validation. public Map<String, String> getSshFingerPrints() {
Map<String, String> fingerprints = new HashMap<>();
KeyPairProvider keyPairProvider = sshServer.getKeyPairProvider();
- if (keyPairProvider != null) {
+ if (keyPairProvider == null) {
+ log.warn("No KeyPairProvider configured in SSH server");
+ return fingerprints;
+ }
+
+ try {
+ var keys = keyPairProvider.loadKeys(null);
+ if (keys == null) {
+ log.warn("No SSH keys loaded from provider");
+ return fingerprints;
+ }
+
+ keys.iterator().forEachRemaining(keyPair -> {
+ if (keyPair != null && keyPair.getPublic() != null) {
+ var fingerprint = HashUtils.getSha256Fingerprint(keyPair.getPublic());
+ if (fingerprint != null && fingerprint.startsWith("SHA256:")) {
+ fingerprints.put(keyPair.getPublic().getAlgorithm(), fingerprint);
+ } else {
+ log.warn("Invalid fingerprint format for key algorithm: {}",
+ keyPair.getPublic().getAlgorithm());
+ }
+ }
+ }); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
catch (IOException | GeneralSecurityException e) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
log.info("Could not load keys from the ssh server while trying to get SSH key fingerprints", e); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
throw new BadRequestException("Could not load keys from the ssh server"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+43
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use more appropriate exception type and enhance error handling BadRequestException is typically used for invalid client input, not server-side issues. Consider using a more appropriate exception type and provide more detailed error information. - log.info("Could not load keys from the ssh server while trying to get SSH key fingerprints", e);
- throw new BadRequestException("Could not load keys from the ssh server");
+ log.error("Failed to load SSH keys: {}", e.getMessage(), e);
+ throw new ServiceException("Failed to retrieve SSH fingerprints due to key loading error", e);
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return fingerprints; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
package de.tum.cit.aet.artemis.programming.web.localvc.ssh; | ||
|
||
import static de.tum.cit.aet.artemis.core.config.Constants.PROFILE_LOCALVC; | ||
|
||
import java.util.Map; | ||
|
||
import org.springframework.context.annotation.Profile; | ||
import org.springframework.http.MediaType; | ||
import org.springframework.http.ResponseEntity; | ||
import org.springframework.web.bind.annotation.GetMapping; | ||
import org.springframework.web.bind.annotation.RequestMapping; | ||
import org.springframework.web.bind.annotation.RestController; | ||
|
||
import de.tum.cit.aet.artemis.core.security.annotations.EnforceAtLeastStudent; | ||
import de.tum.cit.aet.artemis.core.service.feature.Feature; | ||
import de.tum.cit.aet.artemis.core.service.feature.FeatureToggle; | ||
import de.tum.cit.aet.artemis.programming.service.localvc.ssh.SshFingerprintsProviderService; | ||
|
||
/** | ||
* REST controller for managing. | ||
*/ | ||
@Profile(PROFILE_LOCALVC) | ||
@RestController | ||
@RequestMapping("api/") | ||
public class SshFingerprintsProviderResource { | ||
|
||
SshFingerprintsProviderService sshFingerprintsProviderService; | ||
|
||
public SshFingerprintsProviderResource(SshFingerprintsProviderService sshFingerprintsProviderService) { | ||
this.sshFingerprintsProviderService = sshFingerprintsProviderService; | ||
} | ||
|
||
/** | ||
* GET /ssh-fingerprints | ||
* | ||
* @return the SSH fingerprints for the keys a user uses | ||
*/ | ||
@GetMapping(value = "ssh-fingerprints", produces = MediaType.APPLICATION_JSON_VALUE) | ||
@EnforceAtLeastStudent | ||
@FeatureToggle(Feature.Exports) | ||
public ResponseEntity<Map<String, String>> getSshFingerprints() { | ||
return ResponseEntity.ok().body(sshFingerprintsProviderService.getSshFingerPrints()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
<h1 jhiTranslate="artemisApp.userSettings.sshSettingsPage.sshFingerprints"></h1> | ||
|
||
<div class="list-group d-block"> | ||
<!-- Viewing existing key and creating a new key --> | ||
<div class="list-group-item"> | ||
<div class="d-flex flex-wrap"> | ||
<p class="font-medium"> | ||
<span class="mt-4" jhiTranslate="artemisApp.userSettings.sshSettingsPage.fingerprintsExplanation"> </span> | ||
<jhi-documentation-link [documentationType]="documentationType" [displayString]="'artemisApp.userSettings.sshSettingsPage.fingerprintsLearnMore'"> | ||
</jhi-documentation-link> | ||
</p> | ||
</div> | ||
|
||
@if (sshFingerprints && sshFingerprints['RSA']) { | ||
<div class="row small-text"> | ||
<div class="column left"> | ||
{{ 'RSA' }} | ||
</div> | ||
<div class="column right"> | ||
{{ sshFingerprints['RSA'] }} | ||
</div> | ||
</div> | ||
} | ||
Comment on lines
+14
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Refactor repeated fingerprint display blocks The template contains duplicate code blocks for different SSH key types. Consider extracting this into a reusable component or using - @if (sshFingerprints && sshFingerprints['RSA']) {
- <div class="row small-text">
- <div class="column left">
- {{ 'RSA' }}
- </div>
- <div class="column right">
- {{ sshFingerprints['RSA'] }}
- </div>
- </div>
- }
- // ... similar blocks for other types
+ @for (type of ['RSA', 'EdDSA', 'ECDSA', 'EC'] as type) {
+ @if (sshFingerprints && sshFingerprints[type]) {
+ <div class="row small-text">
+ <div class="column left">
+ {{ type === 'EdDSA' ? 'ED25519' : type }}
+ </div>
+ <div class="column right">
+ {{ sshFingerprints[type] }}
+ </div>
+ </div>
+ }
+ } Also applies to: 25-34, 36-45, 47-56 |
||
|
||
@if (sshFingerprints && sshFingerprints['EdDSA']) { | ||
<div class="row small-text"> | ||
<div class="column left"> | ||
{{ 'ED25519' }} | ||
</div> | ||
<div class="column right"> | ||
{{ sshFingerprints['EdDSA'] }} | ||
</div> | ||
</div> | ||
} | ||
|
||
@if (sshFingerprints && sshFingerprints['ECDSA']) { | ||
<div class="row"> | ||
<div class="column left"> | ||
{{ 'ECDSA' }} | ||
</div> | ||
<div class="column right"> | ||
{{ sshFingerprints['ECDSA'] }} | ||
</div> | ||
</div> | ||
} | ||
|
||
@if (sshFingerprints && sshFingerprints['EC']) { | ||
<div class="row"> | ||
<div class="column left"> | ||
{{ 'ECDSA' }} | ||
</div> | ||
<div class="column right"> | ||
{{ sshFingerprints['EC'] }} | ||
</div> | ||
</div> | ||
} | ||
|
||
<div class="d-flex justify-content-between align-items-center mt-4"> | ||
<div></div> | ||
<div> | ||
<a class="btn rounded-btn btn-primary btn-sm" [routerLink]="['..']"> | ||
<span class="jhi-btn__title" style="font-size: small" jhiTranslate="artemisApp.userSettings.sshSettingsPage.back"></span> | ||
</a> | ||
</div> | ||
</div> | ||
</div> | ||
</div> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
.column { | ||
float: left; | ||
padding: 10px; | ||
} | ||
|
||
.left { | ||
width: 15%; | ||
} | ||
|
||
.right { | ||
width: 85%; | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,24 @@ | ||||||||||||||||||||||||||||||||||
import { Component, OnInit, inject } from '@angular/core'; | ||||||||||||||||||||||||||||||||||
import { ButtonSize, ButtonType } from 'app/shared/components/button.component'; | ||||||||||||||||||||||||||||||||||
import { DocumentationType } from 'app/shared/components/documentation-button/documentation-button.component'; | ||||||||||||||||||||||||||||||||||
import { SshUserSettingsFingerprintsService } from 'app/shared/user-settings/ssh-settings/fingerprints/ssh-user-settings-fingerprints.service'; | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
@Component({ | ||||||||||||||||||||||||||||||||||
selector: 'jhi-account-information', | ||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Update component selector to match component name The selector - selector: 'jhi-account-information',
+ selector: 'jhi-ssh-user-settings-fingerprints', 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||
templateUrl: './ssh-user-settings-fingerprints.component.html', | ||||||||||||||||||||||||||||||||||
styleUrls: ['./ssh-user-settings-fingerprints.component.scss', '../ssh-user-settings.component.scss'], | ||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||
export class SshUserSettingsFingerprintsComponent implements OnInit { | ||||||||||||||||||||||||||||||||||
readonly sshUserSettingsService = inject(SshUserSettingsFingerprintsService); | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
protected sshFingerprints?: { [key: string]: string }; | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
readonly documentationType: DocumentationType = 'SshSetup'; | ||||||||||||||||||||||||||||||||||
protected readonly ButtonType = ButtonType; | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
protected readonly ButtonSize = ButtonSize; | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
async ngOnInit() { | ||||||||||||||||||||||||||||||||||
this.sshFingerprints = await this.sshUserSettingsService.getSshFingerprints(); | ||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||
Comment on lines
+21
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add loading state and error handling The component should handle loading states and potential errors from the service. + protected loading = false;
+ protected error?: string;
+
async ngOnInit() {
+ this.loading = true;
+ try {
this.sshFingerprints = await this.sshUserSettingsService.getSshFingerprints();
+ } catch (err) {
+ this.error = this.sshUserSettingsService.error;
+ } finally {
+ this.loading = false;
+ }
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import { Injectable, inject } from '@angular/core'; | ||
import { HttpClient } from '@angular/common/http'; | ||
import { firstValueFrom } from 'rxjs'; | ||
|
||
@Injectable({ providedIn: 'root' }) | ||
export class SshUserSettingsFingerprintsService { | ||
error?: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance error handling for HTTP requests The service currently has an public async getSshFingerprints(): Promise<{ [key: string]: string }> {
- return await firstValueFrom(this.http.get<{ [key: string]: string }>('api/ssh-fingerprints'));
+ try {
+ return await firstValueFrom(this.http.get<{ [key: string]: string }>('api/ssh-fingerprints'));
+ } catch (err) {
+ this.error = err instanceof Error ? err.message : 'Failed to fetch SSH fingerprints';
+ throw err;
+ }
} Also applies to: 11-13 |
||
|
||
private http = inject(HttpClient); | ||
|
||
public async getSshFingerprints(): Promise<{ [key: string]: string }> { | ||
return await firstValueFrom(this.http.get<{ [key: string]: string }>('api/ssh-fingerprints')); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
package de.tum.cit.aet.artemis.programming.icl; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.mockito.Mockito.doReturn; | ||
import static org.mockito.Mockito.doThrow; | ||
|
||
import java.io.IOException; | ||
import java.security.KeyPair; | ||
import java.security.KeyPairGenerator; | ||
import java.util.Collections; | ||
import java.util.Map; | ||
|
||
import org.apache.sshd.common.keyprovider.KeyPairProvider; | ||
import org.apache.sshd.server.SshServer; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.Nested; | ||
import org.junit.jupiter.api.Test; | ||
import org.mockito.Mock; | ||
import org.springframework.boot.test.mock.mockito.MockBean; | ||
import org.springframework.http.HttpStatus; | ||
import org.springframework.security.test.context.support.WithMockUser; | ||
|
||
import de.tum.cit.aet.artemis.programming.service.localvc.ssh.HashUtils; | ||
import de.tum.cit.aet.artemis.shared.base.AbstractSpringIntegrationLocalCILocalVCTest; | ||
|
||
class SshFingerprintsProviderIntegrationTest extends AbstractSpringIntegrationLocalCILocalVCTest { | ||
|
||
private static final String TEST_PREFIX = "sshFingerprintsTest"; | ||
|
||
@MockBean | ||
private SshServer sshServer; | ||
|
||
@Mock | ||
private KeyPairProvider keyPairProvider; | ||
|
||
private String expectedFingerprint; | ||
|
||
@BeforeEach | ||
void setup() throws Exception { | ||
KeyPairGenerator keyPairGenerator = KeyPairGenerator.getInstance("RSA"); | ||
keyPairGenerator.initialize(2048); | ||
KeyPair testKeyPair = keyPairGenerator.generateKeyPair(); | ||
|
||
expectedFingerprint = HashUtils.getSha256Fingerprint(testKeyPair.getPublic()); | ||
doReturn(keyPairProvider).when(sshServer).getKeyPairProvider(); | ||
doReturn(Collections.singleton(testKeyPair)).when(keyPairProvider).loadKeys(null); | ||
} | ||
|
||
@Nested | ||
class SshFingerprintsProvider { | ||
|
||
@Test | ||
@WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") | ||
void shouldReturnFingerprints() throws Exception { | ||
var response = request.get("/api/ssh-fingerprints", HttpStatus.OK, Map.class); | ||
assertThat(response.get("RSA")).isNotNull(); | ||
assertThat(response.get("RSA")).isEqualTo(expectedFingerprint); | ||
} | ||
|
||
@Test | ||
@WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") | ||
void shouldReturnNoFingerprintsWithoutKeyProviderSetup() throws Exception { | ||
doReturn(null).when(sshServer).getKeyPairProvider(); | ||
|
||
var response = request.get("/api/ssh-fingerprints", HttpStatus.OK, Map.class); | ||
assertThat(response.isEmpty()).isTrue(); | ||
} | ||
|
||
@Test | ||
@WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") | ||
void shouldReturnBadRequestWhenLoadKeysThrowsException() throws Exception { | ||
doThrow(new IOException("Test exception")).when(keyPairProvider).loadKeys(null); | ||
|
||
request.get("/api/ssh-fingerprints", HttpStatus.BAD_REQUEST, Map.class); | ||
} | ||
} | ||
Comment on lines
+49
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add test case for unauthorized access The test suite covers successful retrieval, missing key provider, and exception scenarios. Consider adding a test case for unauthorized access to ensure proper security constraints. Add this test method: @Test
void shouldReturnUnauthorizedWithoutUser() throws Exception {
request.get("/api/ssh-fingerprints", HttpStatus.UNAUTHORIZED, Map.class);
} |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add input validation and documentation
The method should include parameter validation and proper JavaDoc documentation.
📝 Committable suggestion