Skip to content

Conversation

@jreidinger
Copy link
Contributor

@jreidinger jreidinger commented Apr 15, 2025

Problem

Using registration server that uses self-signed certificate is not possible.

Solution

In this initial pull request implement question for user if certificate should be imported or not.

Testing

  • Added a new unit test
  • Tested manually

json used to invoke popup:

{
  "product": {
    "id": "SLES",
    "registrationCode": "test",
    "registrationUrl": "https://migration-rmt2.qe.nue2.suse.org"
  }
}

json to test unattended ( will return product not found on server instead of certificate product as I do not have any rmt with SLES16 support)

{
  "product": {
    "id": "SLES",
    "registrationCode": "test",
    "registrationUrl": "https://migration-rmt2.qe.nue2.suse.org"
  },
  "security": {
	  "sslCertificates": [{
		 "fingerprint": "F6:7A:ED:BB:BC:94:CF:55:9D:B3:BA:74:7A:87:05:EF:67:4E:C2:DB",
		"algorithm": "SHA1"
	  }
	 ]
  }
}

Screenshots

localhost_8080_ (23)

Documentation

Remember to look at it from the user's perspective. Yes you have made the compiler happy.
But will the humans even know about your contribution? Sometimes they cannot miss it,
other times they need advertisement and explanation.

Look for relevant sections and adjust:

@jreidinger
Copy link
Contributor Author

@joseivanlopez thanks, changed popup looks really good. So now only rest of backend part for unattended support is missing.

<Popup.Actions>
<QuestionActions
actions={question.options}
defaultAction={question.defaultOption}
Copy link
Contributor

@dgdavid dgdavid Apr 23, 2025

Choose a reason for hiding this comment

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

NP: Probably not in the scope of this PR, but reveleaded here.

I believe we have to look for a way for not having a "primary" action but just the focused one for prompts like this where Agama shouldn't use a call to action. Agama should just prevent users trusting in the self certificate by accident focusing the Reject by default, but not encouraging/inviting users to reject it by using a primary action look&feel.

@jreidinger jreidinger marked this pull request as ready for review April 23, 2025 13:00
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

I have reviewed the Rust part.


pub async fn set_registration_url(&self, url: &String) -> Result<(), ServiceError> {
self.client
.put_void("/software/registration/url", url)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks weird to me to have separate endpoints to set the registration code/email and the URL.

Perhaps in the future we might consider to add the URL to the registration params.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeap, but code+email is together registration...not sure if it should be also part of registration method param and expect that all following registration of addons is forced to be on identical url.

Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

Apart from two minor comments, the Rust part is looking good.

Ok(SSLFingerprint {
fingerprint: value,
algorithm: alg.try_into()?,
algorithm: super::model::SSLFingerprintAlgorithm::from_str(&alg)?,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to import the SSLFingerprintAlgorithm as you did with SSLFingerprint and SecuritryProxy.

let dbus_list: Vec<(&str, &str)> = list
.iter()
.map(|s| (s.algorithm.to_str(), s.fingerprint.as_str()))
.map(|s| (s.algorithm.clone().into(), s.fingerprint.as_str()))
Copy link
Contributor

Choose a reason for hiding this comment

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

You can save the explicit clone if you derive Copy. In an enum, it makes sense.


def self.download(url, insecure: false)
# TODO
# result = Downloader.download(url, insecure: insecure)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is something missing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, it is just for future download of certificate from URL if we decide to implement it. It is using Yast downloader, so I kind of expect that we will do it differently in agama.

Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

Please, add a reference to this PR and you can merge :-)

Co-authored-by: Imobach González Sosa <[email protected]>
@jreidinger jreidinger merged commit cd9efcf into master Apr 24, 2025
11 checks passed
@jreidinger jreidinger deleted the initial_certificates branch April 24, 2025 14:32
imobachgs added a commit that referenced this pull request Apr 25, 2025
The Rust part of the project was introduced just as a command-line
interface. However, it has turned into a full blown HTTP/API server
(with a command-line) and we plan to move more code from Ruby to Rust.

We are learning Rust as we go, so some old decisions might need to be
revisited. I have started by trying to simplify the
[ServiceError](https://github.com/agama-project/agama/blob/70e7d987c9478f1c0ed3d1bf0c851485b9b88d84/rust/agama-lib/src/error.rs#L29-L78)
enum, which represents an error but includes variants for many different
situation (even unrelated ones). We even have an "anyhow" variant!

This first round includes:

- Defining an error for each HTTP client.
- Defining an error for each store.
- Defining an error for the storage, which includes one variant for each
store.
- Remove some `ServiceError` variants.
- Make use of `anyhow` in more places of the binary.

Once we have put everything into a better place, we can decide to
simplify the whole thing if needed. Let's go step by step.

## To do

- [x] Test the changes
- [ ] Convert errors with a single variant into structs (?)
- [x] Adapt changes from
#2270.
- [x] Update the changes file

## Follow-up

- There is another whole category errors that are related to D-Bus.
Let's move them to their own error and finish the `ServiceError`
clean-up.
- The HTTP client constructors do not need to be async.
- The D-Bus clients are the "special cases". Perhaps we should consider
moving from DomainHTTPClient to DomainClient (and DomainClient to
DomainDBusClient).

> [!WARNING]
> Do not merge before #2270.
We need to fix some conflicts first.
ancorgs added a commit that referenced this pull request Apr 25, 2025
## Problem

We recently implemented support for specifying a certificate to be used
during registration. See #2270.

Conversion of the fingerprints in an AutoYaST profile to the new Agama
configuration format is still missing.

## Solution

Implement the conversion of AutoYaST `reg_server_cert_fingerprint` and
`reg_server_cert_fingerprint_type` to the Agama format:

```json
{
  "security": {
    "sslCertificates": [
      { "fingerprint": "01:12:34:45:56:67:78:89:90", "algorithm": "SHA1" }
    ]
  }
}
```

## Testing

Added new unit tests
@imobachgs imobachgs mentioned this pull request May 26, 2025
imobachgs added a commit that referenced this pull request May 26, 2025
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