Skip to content
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

gui: allow to connect to an electrum server w/ a self signed certificate #1500

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,14 @@ jobs:
toolchain: ${{ matrix.toolchain }}
override: true
profile: minimal
- name: Install OpenSSL (windows)
if: matrix.os == 'windows-latest'
run: |
choco install openssl.light --no-progress
echo "C:\Program Files\OpenSSL" >> $env:GITHUB_PATH
echo "C:\Program Files\OpenSSL\bin" >> $env::GITHUB_PATH
echo "OPENSSL_DIR=C:\Program Files\OpenSSL" >> $env:GITHUB_ENV

- name: Test on Rust ${{ matrix.toolchain }} (only Windows)
if: matrix.os == 'windows-latest'
run: cargo test --verbose --no-default-features
Expand Down
65 changes: 60 additions & 5 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions contrib/lianad_config_example.toml
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,13 @@ poll_interval_secs = 30
# In order to connect, it needs the address as a string, which can be
# optionally prefixed with "ssl://" or "tcp://". If omitted, "tcp://"
# will be assumed.
# `validate_domain` field is optional: used in case of SSL connection,
# if set to `false`, internal electrum client will not try to validate
# the domain associated to the certificate: it's useful in case of
# self-signed certificate. Its default value is `true`.
# [electrum_config]
# addr = "127.0.0.1:50001"
# validate_domain = false
#
#
[bitcoind_config]
Expand Down
8 changes: 8 additions & 0 deletions liana-gui/src/app/state/settings/bitcoind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ impl BitcoindSettings {
}
}
}
view::SettingsEditMessage::ValidateDomainEdited(_) => {}
view::SettingsEditMessage::BitcoindRpcAuthTypeSelected(auth_type) => {
if !self.processing {
self.selected_auth_type = auth_type;
Expand Down Expand Up @@ -462,6 +463,7 @@ impl ElectrumSettings {
daemon_config.bitcoin_backend =
Some(lianad::config::BitcoinBackend::Electrum(ElectrumConfig {
addr: self.addr.value.clone(),
validate_domain: self.electrum_config.validate_domain,
}));
self.processing = true;
return Command::perform(async move { daemon_config }, |cfg| {
Expand All @@ -470,6 +472,11 @@ impl ElectrumSettings {
}
}
view::SettingsEditMessage::Clipboard(text) => return clipboard::write(text),
view::SettingsEditMessage::ValidateDomainEdited(b) => {
if !self.processing {
self.electrum_config.validate_domain = b;
}
}
_ => {}
};
Command::none()
Expand All @@ -484,6 +491,7 @@ impl ElectrumSettings {
cache.blockheight,
&self.addr,
self.processing,
self.electrum_config.validate_domain,
)
} else {
view::settings::electrum(
Expand Down
1 change: 1 addition & 0 deletions liana-gui/src/app/view/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ pub enum RemoteBackendSettingsMessage {
pub enum SettingsEditMessage {
Select,
FieldEdited(&'static str, String),
ValidateDomainEdited(bool),
pythcoiner marked this conversation as resolved.
Show resolved Hide resolved
BitcoindRpcAuthTypeSelected(RpcAuthType),
Cancel,
Confirm,
Expand Down
7 changes: 6 additions & 1 deletion liana-gui/src/app/view/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ use crate::{
hw::HardwareWallet,
node::{
bitcoind::{RpcAuthType, RpcAuthValues},
electrum,
electrum::{self, validate_domain_checkbox},
},
};

Expand Down Expand Up @@ -563,6 +563,7 @@ pub fn electrum_edit<'a>(
blockheight: i32,
addr: &form::Value<String>,
processing: bool,
validate_domain: bool,
) -> Element<'a, SettingsEditMessage> {
let mut col = Column::new().spacing(20);
if is_configured_node_type && blockheight != 0 {
Expand Down Expand Up @@ -595,6 +596,9 @@ pub fn electrum_edit<'a>(
.push(separation().width(Length::Fill));
}

let checkbox = validate_domain_checkbox(addr, validate_domain, |b| {
SettingsEditMessage::ValidateDomainEdited(b)
});
col = col.push(
Column::new()
.push(text("Address:").bold().small())
Expand All @@ -606,6 +610,7 @@ pub fn electrum_edit<'a>(
.size(P1_SIZE)
.padding(5),
)
.push_maybe(checkbox)
.push(text(electrum::ADDRESS_NOTES).size(P2_SIZE))
.spacing(5),
);
Expand Down
1 change: 1 addition & 0 deletions liana-gui/src/installer/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ pub enum DefineBitcoind {
#[derive(Debug, Clone)]
pub enum DefineElectrum {
ConfigFieldEdited(electrum::ConfigField, String),
ValidDomainChanged(bool),
Copy link
Collaborator

Choose a reason for hiding this comment

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

As validate_domain is part of the config, I feel that this should somehow also be part of ConfigFieldEdited. Perhaps you can use something like ConfigFieldEdited(electrum::ConfigFieldValue) where ConfigFieldValue is:

pub enum ConfigFieldValue {
    Address(String),
    ValidateDomain(bool),
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ValidDomainChanged(true) is shorter than
ConfigFieldEdited(electrum::ConfigFieldValue(electrum::ConfigFieldValue::ValidateDomain(bool)))
the event system of iced is already overwhelming imho, we should avoid "overwrapping" it only make the codebase less readable & more error prone

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it be just ConfigFieldEdited(electrum::ConfigFieldValue::ValidateDomain(bool))?

And by removing electrum:: it would be just ConfigFieldEdited(ConfigFieldValue::ValidateDomain(bool)).

It doesn't seem too bad, but we can leave it for now. If we have a separate message per field, then we should at some point change ConfigFieldEdited to AddressEdited.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

at this point of the release cycle i'd like to bring this PR quickly to a mergeable state or put it on hold: to avoid everyone testing under the christmas tree 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's OK, I meant it as something to consider in the future.

}

#[derive(Debug, Clone)]
Expand Down
21 changes: 18 additions & 3 deletions liana-gui/src/installer/step/node/electrum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,19 @@ use crate::{
node::electrum::ConfigField,
};

#[derive(Clone, Default)]
#[derive(Clone)]
pub struct DefineElectrum {
address: form::Value<String>,
validate_domain: bool,
}

impl Default for DefineElectrum {
fn default() -> Self {
Self {
address: Default::default(),
validate_domain: true,
}
}
}

impl DefineElectrum {
Expand All @@ -38,6 +48,7 @@ impl DefineElectrum {
crate::node::electrum::is_electrum_address_valid(&value);
}
},
message::DefineElectrum::ValidDomainChanged(v) => self.validate_domain = v,
};
};
Command::none()
Expand All @@ -47,19 +58,23 @@ impl DefineElectrum {
if self.can_try_ping() {
ctx.bitcoin_backend = Some(lianad::config::BitcoinBackend::Electrum(ElectrumConfig {
addr: self.address.value.clone(),
validate_domain: self.validate_domain,
}));
return true;
}
false
}

pub fn view(&self) -> Element<Message> {
view::define_electrum(&self.address)
view::define_electrum(&self.address, self.validate_domain)
}

pub fn ping(&self) -> Result<(), Error> {
let builder = electrum_client::Config::builder();
let config = builder.timeout(Some(3)).build();
let config = builder
.timeout(Some(3))
.validate_domain(self.validate_domain)
.build();
let client = electrum_client::Client::from_config(&self.address.value, config)
.map_err(|e| Error::Electrum(e.to_string()))?;
client
Expand Down
14 changes: 12 additions & 2 deletions liana-gui/src/installer/view/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ use liana_ui::{
widget::*,
};

use crate::node::electrum::validate_domain_checkbox;
use crate::{
hw::{is_compatible_with_tapminiscript, HardwareWallet, UnsupportedReason},
installer::{
Expand Down Expand Up @@ -1134,7 +1135,15 @@ pub fn define_bitcoind<'a>(
.into()
}

pub fn define_electrum<'a>(address: &form::Value<String>) -> Element<'a, Message> {
pub fn define_electrum<'a>(
address: &form::Value<String>,
validate_domain: bool,
) -> Element<'a, Message> {
let checkbox = validate_domain_checkbox(address, validate_domain, |b| {
Message::DefineNode(DefineNode::DefineElectrum(
message::DefineElectrum::ValidDomainChanged(b),
))
});
let col_address = Column::new()
.push(text("Address:").bold())
.push(
Expand All @@ -1150,7 +1159,8 @@ pub fn define_electrum<'a>(address: &form::Value<String>) -> Element<'a, Message
.size(text::P1_SIZE)
.padding(10),
)
.push(text(electrum::ADDRESS_NOTES).size(text::P2_SIZE))
pythcoiner marked this conversation as resolved.
Show resolved Hide resolved
.push_maybe(checkbox)
.push(text(electrum::ADDRESS_NOTES))
.spacing(10);

Column::new().push(col_address).spacing(50).into()
Expand Down
30 changes: 28 additions & 2 deletions liana-gui/src/node/electrum.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
use std::fmt;

use iced::{widget::checkbox, Element, Renderer};
use liana_ui::{component::form, theme::Theme};

#[derive(Debug, PartialEq, Eq, Clone, Copy)]
pub enum ConfigField {
Address,
}

pub const ADDRESS_NOTES: &str = "Note: include \"ssl://\" as a prefix \
for SSL connections. Be aware that self-signed \
SSL certificates are currently not supported.";
for SSL connections.";

pub const VALID_SSL_DOMAIN_NOTES: &str = "Do not validate SSL Domain \
(check this only if you want to use a self-signed certificate)";

impl fmt::Display for ConfigField {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
Expand All @@ -17,6 +22,27 @@ impl fmt::Display for ConfigField {
}
}

pub fn validate_domain_checkbox<'a, F, M>(
addr: &form::Value<String>,
value: bool,
closure: F,
) -> Option<Element<'a, M, Theme, Renderer>>
where
F: 'a + Fn(bool) -> M,
M: 'a,
{
let checkbox = checkbox(VALID_SSL_DOMAIN_NOTES, !value).on_toggle(move |b| closure(!b));
if addr.valid && is_ssl(&addr.value) {
Some(checkbox.into())
} else {
None
}
}

pub fn is_ssl(value: &str) -> bool {
value.starts_with("ssl://")
}

pub fn is_electrum_address_valid(value: &str) -> bool {
let value_noprefix = if value.starts_with("ssl://") {
value.replacen("ssl://", "", 1)
Expand Down
1 change: 1 addition & 0 deletions lianad/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ miniscript = { version = "11.0", features = ["serde", "compiler", "base64"] }
# For Electrum backend. This is the latest version with the same bitcoin version as
# the miniscript dependency.
bdk_electrum = { version = "0.14" }
electrum-client = { version = "0.19", features = ["use-openssl"] }

# Don't reinvent the wheel
dirs = "5.0"
Expand Down
Loading
Loading