* [pbs-devel] [RFC backup 1/6] add tools::serde_filter submodule
2020-11-19 14:56 [pbs-devel] [RFC backup 0/6] Two factor authentication Wolfgang Bumiller
@ 2020-11-19 14:56 ` Wolfgang Bumiller
2020-11-19 14:56 ` [pbs-devel] [RFC backup 2/6] config: add tfa configuration Wolfgang Bumiller
` (5 subsequent siblings)
6 siblings, 0 replies; 23+ messages in thread
From: Wolfgang Bumiller @ 2020-11-19 14:56 UTC (permalink / raw)
To: pbs-devel
can be used to perform filtering at parse time
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
src/tools.rs | 1 +
src/tools/serde_filter.rs | 97 +++++++++++++++++++++++++++++++++++++++
2 files changed, 98 insertions(+)
create mode 100644 src/tools/serde_filter.rs
diff --git a/src/tools.rs b/src/tools.rs
index 08f9d22f..8cc446dd 100644
--- a/src/tools.rs
+++ b/src/tools.rs
@@ -32,6 +32,7 @@ pub mod loopdev;
pub mod lru_cache;
pub mod nom;
pub mod runtime;
+pub mod serde_filter;
pub mod socket;
pub mod statistics;
pub mod subscription;
diff --git a/src/tools/serde_filter.rs b/src/tools/serde_filter.rs
new file mode 100644
index 00000000..b8402696
--- /dev/null
+++ b/src/tools/serde_filter.rs
@@ -0,0 +1,97 @@
+use std::marker::PhantomData;
+
+use serde::Deserialize;
+
+/// Helper to filter data while deserializing it.
+///
+/// An example use case is filtering out expired registration challenges at load time of our TFA
+/// config:
+///
+/// ```
+/// # use proxmox_backup::tools::serde_filter::FilteredVecVisitor;
+/// # use serde::{Deserialize, Deserializer, Serialize};
+/// # const CHALLENGE_TIMEOUT: i64 = 2 * 60;
+/// #[derive(Deserialize)]
+/// struct Challenge {
+/// /// Expiration time as unix epoch.
+/// expires: i64,
+///
+/// // ...other entries...
+/// }
+///
+/// #[derive(Default, Deserialize)]
+/// #[serde(deny_unknown_fields)]
+/// #[serde(rename_all = "kebab-case")]
+/// pub struct TfaUserData {
+/// // ...other entries...
+///
+/// #[serde(skip_serializing_if = "Vec::is_empty", default)]
+/// #[serde(deserialize_with = "filter_expired_registrations")]
+/// registrations: Vec<Challenge>,
+/// }
+///
+/// fn filter_expired_registrations<'de, D>(deserializer: D) -> Result<Vec<Challenge>, D::Error>
+/// where
+/// D: Deserializer<'de>,
+/// {
+/// let expire_before = proxmox::tools::time::epoch_i64() - CHALLENGE_TIMEOUT;
+///
+/// Ok(deserializer.deserialize_seq(
+/// FilteredVecVisitor::new(
+/// "a u2f registration challenge entry",
+/// move |c: &Challenge| c.expires < expire_before,
+/// )
+/// )?)
+/// }
+/// ```
+pub struct FilteredVecVisitor<F, T>
+where
+ F: Fn(&T) -> bool
+{
+ filter: F,
+ expecting: &'static str,
+ _ty: PhantomData<T>,
+}
+
+impl<F, T> FilteredVecVisitor<F, T>
+where
+ F: Fn(&T) -> bool,
+{
+ pub fn new(expecting: &'static str, filter: F) -> Self {
+ Self {
+ filter,
+ expecting,
+ _ty: PhantomData,
+ }
+ }
+}
+
+impl<'de, F, T> serde::de::Visitor<'de> for FilteredVecVisitor<F, T>
+where
+ F: Fn(&T) -> bool,
+ T: Deserialize<'de>,
+{
+ type Value = Vec<T>;
+
+ fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
+ formatter.write_str(self.expecting)
+ }
+
+ fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
+ where
+ A: serde::de::SeqAccess<'de>,
+ {
+ let mut out = match seq.size_hint() {
+ Some(hint) => Vec::with_capacity(hint),
+ None => Vec::new(),
+ };
+
+ while let Some(entry) = seq.next_element::<T>()? {
+ if (self.filter)(&entry) {
+ out.push(entry);
+ }
+ }
+
+ Ok(out)
+ }
+}
--
2.20.1
^ permalink raw reply [flat|nested] 23+ messages in thread* [pbs-devel] [RFC backup 2/6] config: add tfa configuration
2020-11-19 14:56 [pbs-devel] [RFC backup 0/6] Two factor authentication Wolfgang Bumiller
2020-11-19 14:56 ` [pbs-devel] [RFC backup 1/6] add tools::serde_filter submodule Wolfgang Bumiller
@ 2020-11-19 14:56 ` Wolfgang Bumiller
2020-11-19 14:56 ` [pbs-devel] [RFC backup 3/6] api: tfa management and login Wolfgang Bumiller
` (4 subsequent siblings)
6 siblings, 0 replies; 23+ messages in thread
From: Wolfgang Bumiller @ 2020-11-19 14:56 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
src/config.rs | 1 +
src/config/tfa.rs | 643 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 644 insertions(+)
create mode 100644 src/config/tfa.rs
diff --git a/src/config.rs b/src/config.rs
index 6f19da7c..8e95d580 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -21,6 +21,7 @@ pub mod datastore;
pub mod network;
pub mod remote;
pub mod sync;
+pub mod tfa;
pub mod token_shadow;
pub mod user;
pub mod verify;
diff --git a/src/config/tfa.rs b/src/config/tfa.rs
new file mode 100644
index 00000000..da3a4f9a
--- /dev/null
+++ b/src/config/tfa.rs
@@ -0,0 +1,643 @@
+use std::collections::HashMap;
+use std::fs::File;
+use std::time::Duration;
+
+use anyhow::{bail, format_err, Error};
+use serde::{de::Deserializer, Deserialize, Serialize};
+use serde_json::Value;
+
+use proxmox::api::api;
+use proxmox::sys::error::SysError;
+use proxmox::tools::tfa::totp::Totp;
+use proxmox::tools::tfa::u2f;
+use proxmox::tools::uuid::Uuid;
+
+use crate::api2::types::Userid;
+
+/// Mapping of userid to TFA entry.
+pub type TfaUsers = HashMap<Userid, TfaUserData>;
+
+const CONF_FILE: &str = configdir!("/tfa.json");
+const LOCK_FILE: &str = configdir!("/tfa.json.lock");
+const LOCK_TIMEOUT: Duration = Duration::from_secs(5);
+
+/// U2F registration challenges time out after 2 minutes.
+const CHALLENGE_TIMEOUT: i64 = 2 * 60;
+
+#[derive(Deserialize, Serialize)]
+pub struct U2fConfig {
+ appid: String,
+}
+
+#[derive(Default, Deserialize, Serialize)]
+pub struct TfaConfig {
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub u2f: Option<U2fConfig>,
+ #[serde(skip_serializing_if = "TfaUsers::is_empty", default)]
+ pub users: TfaUsers,
+}
+
+/// Heper to get a u2f instance from a u2f config, or `None` if there isn't one configured.
+fn get_u2f(u2f: &Option<U2fConfig>) -> Option<u2f::U2f> {
+ u2f.as_ref().map(|cfg| u2f::U2f::new(cfg.appid.clone(), cfg.appid.clone()))
+}
+
+/// Heper to get a u2f instance from a u2f config.
+// deduplicate error message while working around self-borrow issue
+fn need_u2f(u2f: &Option<U2fConfig>) -> Result<u2f::U2f, Error> {
+ get_u2f(u2f).ok_or_else(|| format_err!("no u2f configuration available"))
+}
+
+impl TfaConfig {
+ fn u2f(&self) -> Option<u2f::U2f> {
+ get_u2f(&self.u2f)
+ }
+
+ fn need_u2f(&self) -> Result<u2f::U2f, Error> {
+ need_u2f(&self.u2f)
+ }
+
+ /// Get a two factor authentication challenge for a user, if the user has TFA set up.
+ pub fn login_challenge(&self, userid: &Userid) -> Result<Option<TfaChallenge>, Error> {
+ match self.users.get(userid) {
+ Some(udata) => udata.challenge(self.u2f().as_ref()),
+ None => Ok(None),
+ }
+ }
+
+ /// Get a u2f registration challenge.
+ fn u2f_registration_challenge(
+ &mut self,
+ user: &Userid,
+ description: String,
+ ) -> Result<String, Error> {
+ let u2f = self.need_u2f()?;
+
+ self.users
+ .entry(user.clone())
+ .or_default()
+ .u2f_registration_challenge(&u2f, description)
+ }
+
+ /// Finish a u2f registration challenge.
+ fn u2f_registration_finish(
+ &mut self,
+ user: &Userid,
+ challenge: &str,
+ response: &str,
+ ) -> Result<String, Error> {
+ let u2f = self.need_u2f()?;
+
+ match self.users.get_mut(user) {
+ Some(user) => user.u2f_registration_finish(&u2f, challenge, response),
+ None => bail!("no such challenge"),
+ }
+ }
+
+ /// Verify a TFA response.
+ fn verify(
+ &mut self,
+ userid: &Userid,
+ challenge: &TfaChallenge,
+ response: TfaResponse,
+ ) -> Result<(), Error> {
+ match self.users.get_mut(userid) {
+ Some(user) => {
+ match response {
+ TfaResponse::Totp(value) => user.verify_totp(&value),
+ TfaResponse::U2f(value) => match &challenge.u2f {
+ Some(challenge) => {
+ let u2f = need_u2f(&self.u2f)?;
+ user.verify_u2f(u2f, &challenge.challenge, value)
+ }
+ None => bail!("no u2f factor available for user '{}'", userid),
+ }
+ TfaResponse::Recovery(value) => user.verify_recovery(&value),
+ }
+ }
+ None => bail!("no 2nd factor available for user '{}'", userid),
+ }
+ }
+}
+
+#[api]
+/// Over the API we only provide this part when querying a user's second factor list.
+#[derive(Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]
+pub struct TfaInfo {
+ /// The id used to reference this entry.
+ pub id: String,
+
+ /// User chosen description for this entry.
+ pub description: String,
+
+ /// Whether this TFA entry is currently enabled.
+ #[serde(skip_serializing_if = "is_default_tfa_enable")]
+ #[serde(default = "default_tfa_enable")]
+ pub enable: bool,
+}
+
+impl TfaInfo {
+ /// For recovery keys we have a fixed entry.
+ pub(crate) fn recovery() -> Self {
+ Self {
+ id: "recovery".to_string(),
+ description: "recovery keys".to_string(),
+ enable: true,
+ }
+ }
+}
+
+/// A TFA entry for a user.
+///
+/// This simply connects a raw registration to a non optional descriptive text chosen by the user.
+#[derive(Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]
+pub struct TfaEntry<T> {
+ #[serde(flatten)]
+ pub info: TfaInfo,
+
+ /// The actual entry.
+ entry: T,
+}
+
+impl<T> TfaEntry<T> {
+ /// Create an entry with a description. The id will be autogenerated.
+ fn new(description: String, entry: T) -> Self {
+ Self {
+ info: TfaInfo {
+ id: Uuid::generate().to_string(),
+ enable: true,
+ description,
+ },
+ entry,
+ }
+ }
+}
+
+/// A u2f registration challenge.
+#[derive(Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]
+pub struct U2fRegistrationChallenge {
+ /// JSON formatted challenge string.
+ challenge: String,
+
+ /// The description chosen by the user for this registration.
+ description: String,
+
+ /// When the challenge was created as unix epoch. They are supposed to be short-lived.
+ created: i64,
+}
+
+impl U2fRegistrationChallenge {
+ pub fn new(challenge: String, description: String) -> Self {
+ Self {
+ challenge,
+ description,
+ created: proxmox::tools::time::epoch_i64(),
+ }
+ }
+
+ fn is_expired(&self, at_epoch: i64) -> bool {
+ self.created < at_epoch
+ }
+}
+
+/// TFA data for a user.
+#[derive(Default, Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]
+#[serde(rename_all = "kebab-case")]
+pub struct TfaUserData {
+ /// Totp keys for a user.
+ #[serde(skip_serializing_if = "Vec::is_empty", default)]
+ pub(crate) totp: Vec<TfaEntry<Totp>>,
+
+ /// Registered u2f tokens for a user.
+ #[serde(skip_serializing_if = "Vec::is_empty", default)]
+ pub(crate) u2f: Vec<TfaEntry<u2f::Registration>>,
+
+ /// Recovery keys. (Unordered OTP values).
+ #[serde(skip_serializing_if = "Vec::is_empty", default)]
+ pub(crate) recovery: Vec<String>,
+
+ /// Active u2f registration challenges for a user.
+ ///
+ /// Expired values are automatically filtered out while parsing the tfa configuration file.
+ #[serde(skip_serializing_if = "Vec::is_empty", default)]
+ #[serde(deserialize_with = "filter_expired_registrations")]
+ u2f_registrations: Vec<U2fRegistrationChallenge>,
+}
+
+/// Serde helper using our `FilteredVecVisitor` to filter out expired entries directly at load
+/// time.
+fn filter_expired_registrations<'de, D>(
+ deserializer: D,
+) -> Result<Vec<U2fRegistrationChallenge>, D::Error>
+where
+ D: Deserializer<'de>,
+{
+ let expire_before = proxmox::tools::time::epoch_i64() - CHALLENGE_TIMEOUT;
+ Ok(
+ deserializer.deserialize_seq(crate::tools::serde_filter::FilteredVecVisitor::new(
+ "a u2f registration challenge entry",
+ move |reg: &U2fRegistrationChallenge| !reg.is_expired(expire_before),
+ ))?,
+ )
+}
+
+impl TfaUserData {
+ /// `true` if no second factors exist
+ pub fn is_empty(&self) -> bool {
+ self.totp.is_empty() && self.u2f.is_empty() && self.recovery.is_empty()
+ }
+
+ /// Find an entry by id, except for the "recovery" entry which we're currently treating
+ /// specially.
+ pub fn find_entry_mut<'a>(&'a mut self, id: &str) -> Option<&'a mut TfaInfo> {
+ for entry in &mut self.totp {
+ if entry.info.id == id {
+ return Some(&mut entry.info);
+ }
+ }
+
+ for entry in &mut self.u2f {
+ if entry.info.id == id {
+ return Some(&mut entry.info);
+ }
+ }
+
+ None
+ }
+
+ /// Create a u2f registration challenge.
+ ///
+ /// The description is required at this point already mostly to better be able to identify such
+ /// challenges in the tfa config file if necessary. The user otherwise has no access to this
+ /// information at this point, as the challenge is identified by its actual challenge data
+ /// instead.
+ fn u2f_registration_challenge(
+ &mut self,
+ u2f: &u2f::U2f,
+ description: String,
+ ) -> Result<String, Error> {
+ let challenge = serde_json::to_string(&u2f.registration_challenge()?)?;
+
+ self.u2f_registrations.push(U2fRegistrationChallenge::new(
+ challenge.clone(),
+ description,
+ ));
+
+ Ok(challenge)
+ }
+
+ /// Finish a u2f registration. The challenge should correspond to an output of
+ /// `u2f_registration_challenge` (which is a stringified `RegistrationChallenge`). The response
+ /// should come directly from the client.
+ fn u2f_registration_finish(
+ &mut self,
+ u2f: &u2f::U2f,
+ challenge: &str,
+ response: &str,
+ ) -> Result<String, Error> {
+ let expire_before = proxmox::tools::time::epoch_i64() - CHALLENGE_TIMEOUT;
+
+ let index = self
+ .u2f_registrations
+ .iter()
+ .position(|r| r.challenge == challenge)
+ .ok_or_else(|| format_err!("no such challenge"))?;
+
+ let reg = &self.u2f_registrations[index];
+ if reg.is_expired(expire_before) {
+ bail!("no such challenge");
+ }
+
+ // the verify call only takes the actual challenge string, so we have to extract it
+ // (u2f::RegistrationChallenge did not always implement Deserialize...)
+ let chobj: Value = serde_json::from_str(challenge)
+ .map_err(|err| format_err!("error parsing original registration challenge: {}", err))?;
+ let challenge = chobj["challenge"]
+ .as_str()
+ .ok_or_else(|| format_err!("invalid registration challenge"))?;
+
+ let (mut reg, description) = match u2f.registration_verify(challenge, response)? {
+ None => bail!("verification failed"),
+ Some(reg) => {
+ let entry = self.u2f_registrations.remove(index);
+ (reg, entry.description)
+ }
+ };
+
+ // we do not care about the attestation certificates, so don't store them
+ reg.certificate.clear();
+
+ let entry = TfaEntry::new(description, reg);
+ let id = entry.info.id.clone();
+ self.u2f.push(entry);
+ Ok(id)
+ }
+
+ /// Generate a generic TFA challenge. See the [`TfaChallenge`] description for details.
+ pub fn challenge(&self, u2f: Option<&u2f::U2f>) -> Result<Option<TfaChallenge>, Error> {
+ if self.is_empty() {
+ return Ok(None);
+ }
+
+ Ok(Some(TfaChallenge {
+ totp: self.totp.iter().any(|e| e.info.enable),
+ recovery: RecoveryState::from_count(self.recovery.len()),
+ u2f: match u2f {
+ Some(u2f) => self.u2f_challenge(u2f)?,
+ None => None,
+ },
+ }))
+ }
+
+ /// Helper to iterate over enabled totp entries.
+ fn enabled_totp_entries(&self) -> impl Iterator<Item = &Totp> {
+ self.totp
+ .iter()
+ .filter_map(|e| {
+ if e.info.enable {
+ Some(&e.entry)
+ } else {
+ None
+ }
+ })
+ }
+
+ /// Helper to iterate over enabled u2f entries.
+ fn enabled_u2f_entries(&self) -> impl Iterator<Item = &u2f::Registration> {
+ self.u2f
+ .iter()
+ .filter_map(|e| {
+ if e.info.enable {
+ Some(&e.entry)
+ } else {
+ None
+ }
+ })
+ }
+
+ /// Generate an optional u2f challenge.
+ fn u2f_challenge(&self, u2f: &u2f::U2f) -> Result<Option<U2fChallenge>, Error> {
+ if self.u2f.is_empty() {
+ return Ok(None);
+ }
+
+ let keys: Vec<u2f::RegisteredKey> = self
+ .enabled_u2f_entries()
+ .map(|registration| registration.key.clone())
+ .collect();
+
+ if keys.is_empty() {
+ return Ok(None);
+ }
+
+ Ok(Some(U2fChallenge {
+ challenge: u2f.auth_challenge()?,
+ keys,
+ }))
+ }
+
+ /// Verify a totp challenge. The `value` should be the totp digits as plain text.
+ fn verify_totp(&self, value: &str) -> Result<(), Error> {
+ let now = std::time::SystemTime::now();
+
+ for entry in self.enabled_totp_entries() {
+ if entry.verify(value, now, -1..=1)?.is_some() {
+ return Ok(());
+ }
+ }
+
+ bail!("totp verification failed");
+ }
+
+ /// Verify a u2f response.
+ fn verify_u2f(
+ &self,
+ u2f: u2f::U2f,
+ challenge: &u2f::AuthChallenge,
+ response: Value,
+ ) -> Result<(), Error> {
+ let response: u2f::AuthResponse = serde_json::from_value(response)
+ .map_err(|err| format_err!("invalid u2f response: {}", err))?;
+
+ if let Some(entry) = self
+ .enabled_u2f_entries()
+ .find(|e| e.key.key_handle == response.key_handle)
+ {
+ if u2f.auth_verify_obj(&entry.public_key, &challenge.challenge, response)?.is_some() {
+ return Ok(());
+ }
+ }
+
+ bail!("u2f verification failed");
+ }
+
+ /// Verify a recovery key.
+ ///
+ /// NOTE: If successful, the key will automatically be removed from the list of available
+ /// recovery keys, so the configuration needs to be saved afterwards!
+ fn verify_recovery(&mut self, value: &str) -> Result<(), Error> {
+ match self.recovery.iter().position(|v| v == value) {
+ Some(idx) => {
+ self.recovery.remove(idx);
+ Ok(())
+ }
+ None => bail!("recovery verification failed"),
+ }
+ }
+
+ /// Add a new set of recovery keys. There can only be 1 set of keys at a time.
+ fn add_recovery(&mut self) -> Result<Vec<String>, Error> {
+ if !self.recovery.is_empty() {
+ bail!("user already has recovery keys");
+ }
+
+ let mut key_data = [0u8; 40]; // 10 keys of 32 bits
+ proxmox::sys::linux::fill_with_random_data(&mut key_data)?;
+ for b in key_data.chunks(4) {
+ self.recovery.push(format!("{:02x}{:02x}{:02x}{:02x}", b[0], b[1], b[2], b[3]));
+ }
+
+ Ok(self.recovery.clone())
+ }
+}
+
+/// Read the TFA entries.
+pub fn read() -> Result<TfaConfig, Error> {
+ let file = match File::open(CONF_FILE) {
+ Ok(file) => file,
+ Err(ref err) if err.not_found() => return Ok(TfaConfig::default()),
+ Err(err) => return Err(err.into()),
+ };
+
+ Ok(serde_json::from_reader(file)?)
+}
+
+/// Requires the write lock to be held.
+pub fn write(data: &TfaConfig) -> Result<(), Error> {
+ let options = proxmox::tools::fs::CreateOptions::new()
+ .perm(nix::sys::stat::Mode::from_bits_truncate(0o0600));
+
+ let json = serde_json::to_vec(data)?;
+ proxmox::tools::fs::replace_file(CONF_FILE, &json, options)
+}
+
+pub fn read_lock() -> Result<File, Error> {
+ proxmox::tools::fs::open_file_locked(LOCK_FILE, LOCK_TIMEOUT, false)
+}
+
+pub fn write_lock() -> Result<File, Error> {
+ proxmox::tools::fs::open_file_locked(LOCK_FILE, LOCK_TIMEOUT, true)
+}
+
+/// Add a TOTP entry for a user. Returns the ID.
+pub fn add_totp(userid: &Userid, description: String, value: Totp) -> Result<String, Error> {
+ let _lock = crate::config::tfa::write_lock();
+ let mut data = read()?;
+ let entry = TfaEntry::new(description, value);
+ let id = entry.info.id.clone();
+ data.users
+ .entry(userid.clone())
+ .or_default()
+ .totp
+ .push(entry);
+ write(&data)?;
+ Ok(id)
+}
+
+/// Add recovery tokens for the user. Returns the token list.
+pub fn add_recovery(userid: &Userid) -> Result<Vec<String>, Error> {
+ let _lock = crate::config::tfa::write_lock();
+
+ let mut data = read()?;
+ let out = data.users.entry(userid.clone()).or_default().add_recovery()?;
+ write(&data)?;
+ Ok(out)
+}
+
+/// Add a u2f registration challenge for a user.
+pub fn add_u2f_registration(userid: &Userid, description: String) -> Result<String, Error> {
+ let _lock = crate::config::tfa::write_lock();
+ let mut data = read()?;
+ let challenge = data.u2f_registration_challenge(userid, description)?;
+ write(&data)?;
+ Ok(challenge)
+}
+
+/// Finish a u2f registration challenge for a user.
+pub fn finish_u2f_registration(
+ userid: &Userid,
+ challenge: &str,
+ response: &str,
+) -> Result<String, Error> {
+ let _lock = crate::config::tfa::write_lock();
+ let mut data = read()?;
+ let challenge = data.u2f_registration_finish(userid, challenge, response)?;
+ write(&data)?;
+ Ok(challenge)
+}
+
+/// Verify a TFA challenge.
+pub fn verify_challenge(
+ userid: &Userid,
+ challenge: &TfaChallenge,
+ response: TfaResponse,
+) -> Result<(), Error> {
+ let _lock = crate::config::tfa::write_lock();
+ let mut data = read()?;
+ data.verify(userid, challenge, response)?;
+ write(&data)?;
+ Ok(())
+}
+
+/// Used to inform the user about the recovery code status.
+#[derive(Clone, Copy, Eq, PartialEq, Deserialize, Serialize)]
+#[serde(rename_all = "kebab-case")]
+pub enum RecoveryState {
+ Unavailable,
+ Low,
+ Available,
+}
+
+impl RecoveryState {
+ fn from_count(count: usize) -> Self {
+ match count {
+ 0 => RecoveryState::Unavailable,
+ 1..=3 => RecoveryState::Low,
+ _ => RecoveryState::Available,
+ }
+ }
+
+ // serde needs `&self` but this is a tiny Copy type, so we mark this as inline
+ #[inline]
+ fn is_unavailable(&self) -> bool {
+ *self == RecoveryState::Unavailable
+ }
+}
+
+impl Default for RecoveryState {
+ fn default() -> Self {
+ RecoveryState::Unavailable
+ }
+}
+
+/// When sending a TFA challenge to the user, we include information about what kind of challenge
+/// the user may perform. If u2f devices are available, a u2f challenge will be included.
+#[derive(Deserialize, Serialize)]
+#[serde(rename_all = "kebab-case")]
+pub struct TfaChallenge {
+ /// True if the user has TOTP devices.
+ totp: bool,
+
+ /// Whether there are recovery keys available.
+ #[serde(skip_serializing_if = "RecoveryState::is_unavailable", default)]
+ recovery: RecoveryState,
+
+ /// If the user has any u2f tokens registered, this will contain the U2F challenge data.
+ #[serde(skip_serializing_if = "Option::is_none")]
+ u2f: Option<U2fChallenge>,
+}
+
+/// Data used for u2f challenges.
+#[derive(Deserialize, Serialize)]
+pub struct U2fChallenge {
+ /// AppID and challenge data.
+ challenge: u2f::AuthChallenge,
+
+ /// Available tokens/keys.
+ keys: Vec<u2f::RegisteredKey>,
+}
+
+/// A user's response to a TFA challenge.
+pub enum TfaResponse {
+ Totp(String),
+ U2f(Value),
+ Recovery(String),
+}
+
+impl std::str::FromStr for TfaResponse {
+ type Err = Error;
+
+ fn from_str(s: &str) -> Result<Self, Error> {
+ Ok(if s.starts_with("totp:") {
+ TfaResponse::Totp(s[5..].to_string())
+ } else if s.starts_with("u2f:") {
+ TfaResponse::U2f(serde_json::from_str(&s[4..])?)
+ } else if s.starts_with("recovery:") {
+ TfaResponse::Recovery(s[9..].to_string())
+ } else {
+ bail!("invalid tfa response");
+ })
+ }
+}
+
+const fn default_tfa_enable() -> bool {
+ true
+}
+
+const fn is_default_tfa_enable(v: &bool) -> bool {
+ *v
+}
--
2.20.1
^ permalink raw reply [flat|nested] 23+ messages in thread* [pbs-devel] [RFC backup 3/6] api: tfa management and login
2020-11-19 14:56 [pbs-devel] [RFC backup 0/6] Two factor authentication Wolfgang Bumiller
2020-11-19 14:56 ` [pbs-devel] [RFC backup 1/6] add tools::serde_filter submodule Wolfgang Bumiller
2020-11-19 14:56 ` [pbs-devel] [RFC backup 2/6] config: add tfa configuration Wolfgang Bumiller
@ 2020-11-19 14:56 ` Wolfgang Bumiller
2020-11-19 14:56 ` [pbs-devel] [RFC backup 4/6] depend on libjs-qrcodejs Wolfgang Bumiller
` (3 subsequent siblings)
6 siblings, 0 replies; 23+ messages in thread
From: Wolfgang Bumiller @ 2020-11-19 14:56 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
Cargo.toml | 1 +
src/api2/access.rs | 171 ++++++++-----
src/api2/access/tfa.rs | 567 +++++++++++++++++++++++++++++++++++++++++
src/config/tfa.rs | 474 ++++++++++++++++++++++++++++++----
src/server.rs | 2 +
src/server/rest.rs | 5 +-
src/server/ticket.rs | 77 ++++++
7 files changed, 1185 insertions(+), 112 deletions(-)
create mode 100644 src/api2/access/tfa.rs
create mode 100644 src/server/ticket.rs
diff --git a/Cargo.toml b/Cargo.toml
index 87aa9cba..1a26bbd0 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -68,6 +68,7 @@ udev = ">= 0.3, <0.5"
url = "2.1"
#valgrind_request = { git = "https://github.com/edef1c/libvalgrind_request", version = "1.1.0", optional = true }
walkdir = "2"
+webauthn-rs = "0.2.5"
xdg = "2.2"
zstd = { version = "0.4", features = [ "bindgen" ] }
nom = "5.1"
diff --git a/src/api2/access.rs b/src/api2/access.rs
index 3b59b3d3..a44d21a2 100644
--- a/src/api2/access.rs
+++ b/src/api2/access.rs
@@ -4,33 +4,46 @@ use serde_json::{json, Value};
use std::collections::HashMap;
use std::collections::HashSet;
-use proxmox::api::{api, RpcEnvironment, Permission};
use proxmox::api::router::{Router, SubdirMap};
-use proxmox::{sortable, identity};
+use proxmox::api::{api, Permission, RpcEnvironment};
use proxmox::{http_err, list_subdirs_api_method};
+use proxmox::{identity, sortable};
-use crate::tools::ticket::{self, Empty, Ticket};
-use crate::auth_helpers::*;
use crate::api2::types::*;
+use crate::auth_helpers::*;
+use crate::server::ticket::ApiTicket;
+use crate::tools::ticket::{self, Empty, Ticket};
use crate::config::acl as acl_config;
-use crate::config::acl::{PRIVILEGES, PRIV_SYS_AUDIT, PRIV_PERMISSIONS_MODIFY};
+use crate::config::acl::{PRIVILEGES, PRIV_PERMISSIONS_MODIFY, PRIV_SYS_AUDIT};
use crate::config::cached_user_info::CachedUserInfo;
+use crate::config::tfa::TfaChallenge;
-pub mod user;
-pub mod domain;
pub mod acl;
+pub mod domain;
pub mod role;
+pub mod tfa;
+pub mod user;
+
+enum AuthResult {
+ /// Successful authentication which does not require a new ticket.
+ Success,
+
+ /// Successful authentication which requires a ticket to be created.
+ CreateTicket,
+
+ /// A partial ticket which requires a 2nd factor will be created.
+ Partial(TfaChallenge),
+}
-/// returns Ok(true) if a ticket has to be created
-/// and Ok(false) if not
fn authenticate_user(
userid: &Userid,
password: &str,
path: Option<String>,
privs: Option<String>,
port: Option<u16>,
-) -> Result<bool, Error> {
+ tfa_challenge: Option<String>,
+) -> Result<AuthResult, Error> {
let user_info = CachedUserInfo::new()?;
let auth_id = Authid::from(userid.clone());
@@ -38,12 +51,16 @@ fn authenticate_user(
bail!("user account disabled or expired.");
}
+ if let Some(tfa_challenge) = tfa_challenge {
+ return authenticate_2nd(userid, &tfa_challenge, password);
+ }
+
if password.starts_with("PBS:") {
if let Ok(ticket_userid) = Ticket::<Userid>::parse(password)
.and_then(|ticket| ticket.verify(public_auth_key(), "PBS", None))
{
if *userid == ticket_userid {
- return Ok(true);
+ return Ok(AuthResult::CreateTicket);
}
bail!("ticket login failed - wrong userid");
}
@@ -53,17 +70,17 @@ fn authenticate_user(
}
let path = path.ok_or_else(|| format_err!("missing path for termproxy ticket"))?;
- let privilege_name = privs
- .ok_or_else(|| format_err!("missing privilege name for termproxy ticket"))?;
+ let privilege_name =
+ privs.ok_or_else(|| format_err!("missing privilege name for termproxy ticket"))?;
let port = port.ok_or_else(|| format_err!("missing port for termproxy ticket"))?;
- if let Ok(Empty) = Ticket::parse(password)
- .and_then(|ticket| ticket.verify(
+ if let Ok(Empty) = Ticket::parse(password).and_then(|ticket| {
+ ticket.verify(
public_auth_key(),
ticket::TERM_PREFIX,
Some(&ticket::term_aad(userid, &path, port)),
- ))
- {
+ )
+ }) {
for (name, privilege) in PRIVILEGES {
if *name == privilege_name {
let mut path_vec = Vec::new();
@@ -73,7 +90,7 @@ fn authenticate_user(
}
}
user_info.check_privs(&auth_id, &path_vec, *privilege, false)?;
- return Ok(false);
+ return Ok(AuthResult::Success);
}
}
@@ -81,8 +98,26 @@ fn authenticate_user(
}
}
- let _ = crate::auth::authenticate_user(userid, password)?;
- Ok(true)
+ let _: () = crate::auth::authenticate_user(userid, password)?;
+
+ Ok(match crate::config::tfa::login_challenge(userid)? {
+ None => AuthResult::CreateTicket,
+ Some(challenge) => AuthResult::Partial(challenge),
+ })
+}
+
+fn authenticate_2nd(
+ userid: &Userid,
+ challenge_ticket: &str,
+ response: &str,
+) -> Result<AuthResult, Error> {
+ let challenge: TfaChallenge = Ticket::<ApiTicket>::parse(&challenge_ticket)?
+ .verify_with_time_frame(public_auth_key(), "PBS", Some(userid.as_str()), -120..240)?
+ .require_partial()?;
+
+ let _: () = crate::config::tfa::verify_challenge(userid, &challenge, response.parse()?)?;
+
+ Ok(AuthResult::CreateTicket)
}
#[api(
@@ -109,6 +144,11 @@ fn authenticate_user(
description: "Port for verifying terminal tickets.",
optional: true,
},
+ "tfa-challenge": {
+ type: String,
+ description: "The signed TFA challenge string the user wants to respond to.",
+ optional: true,
+ },
},
},
returns: {
@@ -141,15 +181,18 @@ fn create_ticket(
path: Option<String>,
privs: Option<String>,
port: Option<u16>,
+ tfa_challenge: Option<String>,
rpcenv: &mut dyn RpcEnvironment,
) -> Result<Value, Error> {
- match authenticate_user(&username, &password, path, privs, port) {
- Ok(true) => {
- let ticket = Ticket::new("PBS", &username)?.sign(private_auth_key(), None)?;
-
+ match authenticate_user(&username, &password, path, privs, port, tfa_challenge) {
+ Ok(AuthResult::Success) => Ok(json!({ "username": username })),
+ Ok(AuthResult::CreateTicket) => {
+ let api_ticket = ApiTicket::full(username.clone());
+ let ticket = Ticket::new("PBS", &api_ticket)?.sign(private_auth_key(), None)?;
let token = assemble_csrf_prevention_token(csrf_secret(), &username);
- crate::server::rest::auth_logger()?.log(format!("successful auth for user '{}'", username));
+ crate::server::rest::auth_logger()?
+ .log(format!("successful auth for user '{}'", username));
Ok(json!({
"username": username,
@@ -157,9 +200,15 @@ fn create_ticket(
"CSRFPreventionToken": token,
}))
}
- Ok(false) => Ok(json!({
- "username": username,
- })),
+ Ok(AuthResult::Partial(challenge)) => {
+ let api_ticket = ApiTicket::partial(challenge);
+ let ticket = Ticket::new("PBS", &api_ticket)?
+ .sign(private_auth_key(), Some(username.as_str()))?;
+ Ok(json!({
+ "username": username,
+ "ticket": ticket,
+ }))
+ }
Err(err) => {
let client_ip = match rpcenv.get_client_ip().map(|addr| addr.ip()) {
Some(ip) => format!("{}", ip),
@@ -206,7 +255,6 @@ fn change_password(
password: String,
rpcenv: &mut dyn RpcEnvironment,
) -> Result<Value, Error> {
-
let current_user: Userid = rpcenv
.get_auth_id()
.ok_or_else(|| format_err!("unknown user"))?
@@ -215,12 +263,16 @@ fn change_password(
let mut allowed = userid == current_user;
- if userid == "root@pam" { allowed = true; }
+ if userid == "root@pam" {
+ allowed = true;
+ }
if !allowed {
let user_info = CachedUserInfo::new()?;
let privs = user_info.lookup_privs(¤t_auth, &[]);
- if (privs & PRIV_PERMISSIONS_MODIFY) != 0 { allowed = true; }
+ if (privs & PRIV_PERMISSIONS_MODIFY) != 0 {
+ allowed = true;
+ }
}
if !allowed {
@@ -277,12 +329,13 @@ pub fn list_permissions(
auth_id
} else if auth_id.is_token()
&& !current_auth_id.is_token()
- && auth_id.user() == current_auth_id.user() {
+ && auth_id.user() == current_auth_id.user()
+ {
auth_id
} else {
bail!("not allowed to list permissions of {}", auth_id);
}
- },
+ }
None => current_auth_id,
}
} else {
@@ -292,11 +345,10 @@ pub fn list_permissions(
}
};
-
fn populate_acl_paths(
mut paths: HashSet<String>,
node: acl_config::AclTreeNode,
- path: &str
+ path: &str,
) -> HashSet<String> {
for (sub_path, child_node) in node.children {
let sub_path = format!("{}/{}", path, &sub_path);
@@ -311,7 +363,7 @@ pub fn list_permissions(
let mut paths = HashSet::new();
paths.insert(path);
paths
- },
+ }
None => {
let mut paths = HashSet::new();
@@ -326,31 +378,35 @@ pub fn list_permissions(
paths.insert("/system".to_string());
paths
- },
+ }
};
- let map = paths
- .into_iter()
- .fold(HashMap::new(), |mut map: HashMap<String, HashMap<String, bool>>, path: String| {
+ let map = paths.into_iter().fold(
+ HashMap::new(),
+ |mut map: HashMap<String, HashMap<String, bool>>, path: String| {
let split_path = acl_config::split_acl_path(path.as_str());
let (privs, propagated_privs) = user_info.lookup_privs_details(&auth_id, &split_path);
match privs {
0 => map, // Don't leak ACL paths where we don't have any privileges
_ => {
- let priv_map = PRIVILEGES
- .iter()
- .fold(HashMap::new(), |mut priv_map, (name, value)| {
- if value & privs != 0 {
- priv_map.insert(name.to_string(), value & propagated_privs != 0);
- }
- priv_map
- });
+ let priv_map =
+ PRIVILEGES
+ .iter()
+ .fold(HashMap::new(), |mut priv_map, (name, value)| {
+ if value & privs != 0 {
+ priv_map
+ .insert(name.to_string(), value & propagated_privs != 0);
+ }
+ priv_map
+ });
map.insert(path, priv_map);
map
- },
- }});
+ }
+ }
+ },
+ );
Ok(map)
}
@@ -358,21 +414,16 @@ pub fn list_permissions(
#[sortable]
const SUBDIRS: SubdirMap = &sorted!([
("acl", &acl::ROUTER),
+ ("password", &Router::new().put(&API_METHOD_CHANGE_PASSWORD)),
(
- "password", &Router::new()
- .put(&API_METHOD_CHANGE_PASSWORD)
- ),
- (
- "permissions", &Router::new()
- .get(&API_METHOD_LIST_PERMISSIONS)
- ),
- (
- "ticket", &Router::new()
- .post(&API_METHOD_CREATE_TICKET)
+ "permissions",
+ &Router::new().get(&API_METHOD_LIST_PERMISSIONS)
),
+ ("ticket", &Router::new().post(&API_METHOD_CREATE_TICKET)),
("domains", &domain::ROUTER),
("roles", &role::ROUTER),
("users", &user::ROUTER),
+ ("tfa", &tfa::ROUTER),
]);
pub const ROUTER: Router = Router::new()
diff --git a/src/api2/access/tfa.rs b/src/api2/access/tfa.rs
new file mode 100644
index 00000000..68ab810a
--- /dev/null
+++ b/src/api2/access/tfa.rs
@@ -0,0 +1,567 @@
+use anyhow::{bail, format_err, Error};
+use serde::{Deserialize, Serialize};
+use serde_json::Value;
+
+use proxmox::api::{api, Permission, Router, RpcEnvironment};
+use proxmox::tools::tfa::totp::Totp;
+use proxmox::{http_bail, http_err};
+
+use crate::api2::types::{Authid, Userid, PASSWORD_SCHEMA};
+use crate::config::acl::{PRIV_PERMISSIONS_MODIFY, PRIV_SYS_AUDIT};
+use crate::config::cached_user_info::CachedUserInfo;
+use crate::config::tfa::{TfaInfo, TfaUserData};
+
+/// Perform first-factor (password) authentication only. Ignore password for the root user. Make
+/// sure a user can only update its own account!
+fn tfa_update_auth(
+ rpcenv: &mut dyn RpcEnvironment,
+ userid: &Userid,
+ password: Option<String>,
+) -> Result<(), Error> {
+ let authid: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+
+ if authid.user() == Userid::root_userid() {
+ return Ok(());
+ }
+
+ let password = password.ok_or_else(|| format_err!("missing password"))?;
+ let _: () = crate::auth::authenticate_user(&userid, &password)?;
+
+ Ok(())
+}
+
+#[api]
+/// A TFA entry type.
+#[derive(Deserialize, Serialize)]
+#[serde(rename_all = "lowercase")]
+pub enum TfaType {
+ /// A TOTP entry type.
+ Totp,
+ /// A U2F token entry.
+ U2f,
+ /// A Webauthn token entry.
+ Webauthn,
+ /// Recovery tokens.
+ Recovery,
+}
+
+#[api(
+ properties: {
+ type: { type: TfaType },
+ info: { type: TfaInfo },
+ },
+)]
+/// A TFA entry for a user.
+#[derive(Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]
+pub struct TypedTfaInfo {
+ #[serde(rename = "type")]
+ pub ty: TfaType,
+
+ #[serde(flatten)]
+ pub info: TfaInfo,
+}
+
+fn to_data(data: TfaUserData) -> Vec<TypedTfaInfo> {
+ let mut out = Vec::with_capacity(
+ data.totp.len()
+ + data.u2f.len()
+ + data.webauthn.len()
+ + if !data.recovery.is_empty() { 1 } else { 0 },
+ );
+ if !data.recovery.is_empty() {
+ out.push(TypedTfaInfo {
+ ty: TfaType::Recovery,
+ info: TfaInfo::recovery(),
+ })
+ }
+ for entry in data.totp {
+ out.push(TypedTfaInfo {
+ ty: TfaType::Totp,
+ info: entry.info,
+ });
+ }
+ for entry in data.webauthn {
+ out.push(TypedTfaInfo {
+ ty: TfaType::Webauthn,
+ info: entry.info,
+ });
+ }
+ for entry in data.u2f {
+ out.push(TypedTfaInfo {
+ ty: TfaType::U2f,
+ info: entry.info,
+ });
+ }
+ out
+}
+
+#[api(
+ protected: true,
+ input: {
+ properties: { userid: { type: Userid } },
+ },
+ access: {
+ permission: &Permission::Or(&[
+ &Permission::Privilege(&["access", "users"], PRIV_PERMISSIONS_MODIFY, false),
+ &Permission::UserParam("userid"),
+ ]),
+ },
+)]
+/// Add a TOTP secret to the user.
+pub fn list_user_tfa(userid: Userid) -> Result<Vec<TypedTfaInfo>, Error> {
+ let _lock = crate::config::tfa::read_lock()?;
+
+ Ok(match crate::config::tfa::read()?.users.remove(&userid) {
+ Some(data) => to_data(data),
+ None => Vec::new(),
+ })
+}
+
+#[api(
+ protected: true,
+ input: {
+ properties: {
+ userid: { type: Userid },
+ id: { description: "the tfa entry id" }
+ },
+ },
+ access: {
+ permission: &Permission::Or(&[
+ &Permission::Privilege(&["access", "users"], PRIV_PERMISSIONS_MODIFY, false),
+ &Permission::UserParam("userid"),
+ ]),
+ },
+)]
+/// Get a single TFA entry.
+pub fn get_tfa(userid: Userid, id: String) -> Result<TypedTfaInfo, Error> {
+ let _lock = crate::config::tfa::read_lock()?;
+
+ if let Some(user_data) = crate::config::tfa::read()?.users.remove(&userid) {
+ if id == "recovery" {
+ if !user_data.recovery.is_empty() {
+ return Ok(TypedTfaInfo {
+ ty: TfaType::Recovery,
+ info: TfaInfo::recovery(),
+ });
+ }
+ } else {
+ for tfa in user_data.totp {
+ if tfa.info.id == id {
+ return Ok(TypedTfaInfo {
+ ty: TfaType::Totp,
+ info: tfa.info,
+ });
+ }
+ }
+
+ for tfa in user_data.webauthn {
+ if tfa.info.id == id {
+ return Ok(TypedTfaInfo {
+ ty: TfaType::Webauthn,
+ info: tfa.info,
+ });
+ }
+ }
+
+ for tfa in user_data.u2f {
+ if tfa.info.id == id {
+ return Ok(TypedTfaInfo {
+ ty: TfaType::U2f,
+ info: tfa.info,
+ });
+ }
+ }
+ }
+ }
+
+ http_bail!(NOT_FOUND, "no such tfa entry: {}/{}", userid, id);
+}
+
+#[api(
+ protected: true,
+ input: {
+ properties: {
+ userid: { type: Userid },
+ id: {
+ description: "the tfa entry id",
+ },
+ password: {
+ schema: PASSWORD_SCHEMA,
+ optional: true,
+ },
+ },
+ },
+ access: {
+ permission: &Permission::Or(&[
+ &Permission::Privilege(&["access", "users"], PRIV_PERMISSIONS_MODIFY, false),
+ &Permission::UserParam("userid"),
+ ]),
+ },
+)]
+/// Get a single TFA entry.
+pub fn delete_tfa(
+ userid: Userid,
+ id: String,
+ password: Option<String>,
+ rpcenv: &mut dyn RpcEnvironment,
+) -> Result<(), Error> {
+ tfa_update_auth(rpcenv, &userid, password)?;
+
+ let _lock = crate::config::tfa::write_lock()?;
+
+ let mut data = crate::config::tfa::read()?;
+
+ let user_data = data
+ .users
+ .get_mut(&userid)
+ .ok_or_else(|| http_err!(NOT_FOUND, "no such entry: {}/{}", userid, id))?;
+
+ let found = if id == "recovery" {
+ let found = !user_data.recovery.is_empty();
+ user_data.recovery.clear();
+ found
+ } else if let Some(i) = user_data.totp.iter().position(|entry| entry.info.id == id) {
+ user_data.totp.remove(i);
+ true
+ } else if let Some(i) = user_data
+ .webauthn
+ .iter()
+ .position(|entry| entry.info.id == id)
+ {
+ user_data.webauthn.remove(i);
+ true
+ } else if let Some(i) = user_data.u2f.iter().position(|entry| entry.info.id == id) {
+ user_data.u2f.remove(i);
+ true
+ } else {
+ false
+ };
+
+ if !found {
+ http_bail!(NOT_FOUND, "no such tfa entry: {}/{}", userid, id);
+ }
+
+ if user_data.is_empty() {
+ data.users.remove(&userid);
+ }
+
+ crate::config::tfa::write(&data)?;
+
+ Ok(())
+}
+
+#[api(
+ properties: {
+ "userid": { type: Userid },
+ "entries": {
+ type: Array,
+ items: { type: TypedTfaInfo },
+ },
+ },
+)]
+#[derive(Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]
+/// Over the API we only provide the descriptions for TFA data.
+pub struct TfaUser {
+ /// The type of TFA entry this is referring to.
+ userid: Userid,
+
+ /// User chosen description for this entry.
+ entries: Vec<TypedTfaInfo>,
+}
+
+#[api(
+ protected: true,
+ input: {
+ properties: {},
+ },
+ access: {
+ permission: &Permission::Anybody,
+ description: "Returns all or just the logged-in user, depending on privileges.",
+ },
+)]
+/// List user TFA configuration.
+pub fn list_tfa(rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+ let authid: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+ let user_info = CachedUserInfo::new()?;
+
+ let top_level_privs = user_info.lookup_privs(&authid, &["access", "users"]);
+ let top_level_allowed = (top_level_privs & PRIV_SYS_AUDIT) != 0;
+
+ let _lock = crate::config::tfa::read_lock()?;
+ let tfa_data = crate::config::tfa::read()?.users;
+
+ let mut out = Vec::<TfaUser>::new();
+ if top_level_allowed {
+ for (user, data) in tfa_data {
+ out.push(TfaUser {
+ userid: user,
+ entries: to_data(data),
+ });
+ }
+ } else {
+ if let Some(data) = { tfa_data }.remove(authid.user()) {
+ out.push(TfaUser {
+ userid: authid.into(),
+ entries: to_data(data),
+ });
+ }
+ }
+
+ Ok(serde_json::to_value(out)?)
+}
+
+#[api(
+ properties: {
+ recovery: {
+ description: "A list of recovery codes as integers.",
+ type: Array,
+ items: {
+ type: Integer,
+ description: "A one-time usable recovery code entry.",
+ },
+ },
+ },
+)]
+/// The result returned when adding TFA entries to a user.
+#[derive(Default, Serialize)]
+struct TfaUpdateInfo {
+ /// The id if a newly added TFA entry.
+ id: Option<String>,
+
+ /// When adding u2f entries, this contains a challenge the user must respond to in order to
+ /// finish the registration.
+ #[serde(skip_serializing_if = "Option::is_none")]
+ challenge: Option<String>,
+
+ /// When adding recovery codes, this contains the list of codes to be displayed to the user
+ /// this one time.
+ #[serde(skip_serializing_if = "Vec::is_empty", default)]
+ recovery: Vec<String>,
+}
+
+impl TfaUpdateInfo {
+ fn id(id: String) -> Self {
+ Self {
+ id: Some(id),
+ ..Default::default()
+ }
+ }
+}
+
+#[api(
+ protected: true,
+ input: {
+ properties: {
+ userid: { type: Userid },
+ description: {
+ description: "A description to distinguish multiple entries from one another",
+ type: String,
+ max_length: 255,
+ optional: true,
+ },
+ "type": {
+ description: "The TFA type to add.",
+ type: TfaType,
+ },
+ totp: {
+ description: "A totp URI.",
+ optional: true,
+ },
+ value: {
+ description:
+ "The current value for the provided totp URI, or a Webauthn/U2F challenge response",
+ optional: true,
+ },
+ challenge: {
+ description: "When responding to a u2f challenge: the original challenge string",
+ optional: true,
+ },
+ password: {
+ schema: PASSWORD_SCHEMA,
+ optional: true,
+ },
+ },
+ },
+ returns: { type: TfaUpdateInfo },
+ access: {
+ permission: &Permission::Or(&[
+ &Permission::Privilege(&["access", "users"], PRIV_PERMISSIONS_MODIFY, false),
+ &Permission::UserParam("userid"),
+ ]),
+ },
+)]
+/// Add a TFA entry to the user.
+fn add_tfa_entry(
+ userid: Userid,
+ description: Option<String>,
+ totp: Option<String>,
+ value: Option<String>,
+ challenge: Option<String>,
+ password: Option<String>,
+ mut params: Value, // FIXME: once api macro supports raw parameters names, use `r#type`
+ rpcenv: &mut dyn RpcEnvironment,
+) -> Result<TfaUpdateInfo, Error> {
+ tfa_update_auth(rpcenv, &userid, password)?;
+
+ let tfa_type: TfaType = serde_json::from_value(params["type"].take())?;
+
+ let need_description =
+ move || description.ok_or_else(|| format_err!("'description' is required for new entries"));
+
+ match tfa_type {
+ TfaType::Totp => match (totp, value) {
+ (Some(totp), Some(value)) => {
+ if challenge.is_some() {
+ bail!("'challenge' parameter is invalid for 'totp' entries");
+ }
+ let description = need_description()?;
+
+ let totp: Totp = totp.parse()?;
+ if totp
+ .verify(&value, std::time::SystemTime::now(), -1..=1)?
+ .is_none()
+ {
+ bail!("failed to verify TOTP challenge");
+ }
+ crate::config::tfa::add_totp(&userid, description, totp).map(TfaUpdateInfo::id)
+ }
+ _ => bail!("'totp' type requires both 'totp' and 'value' parameters"),
+ },
+ TfaType::Webauthn => {
+ if totp.is_some() {
+ bail!("'totp' parameter is invalid for 'totp' entries");
+ }
+
+ match challenge {
+ None => crate::config::tfa::add_webauthn_registration(&userid, need_description()?)
+ .map(|c| TfaUpdateInfo {
+ challenge: Some(c),
+ ..Default::default()
+ }),
+ Some(challenge) => {
+ let value = value.ok_or_else(|| {
+ format_err!(
+ "missing 'value' parameter (webauthn challenge response missing)"
+ )
+ })?;
+ crate::config::tfa::finish_webauthn_registration(&userid, &challenge, &value)
+ .map(TfaUpdateInfo::id)
+ }
+ }
+ }
+ TfaType::U2f => {
+ if totp.is_some() {
+ bail!("'totp' parameter is invalid for 'totp' entries");
+ }
+
+ match challenge {
+ None => crate::config::tfa::add_u2f_registration(&userid, need_description()?).map(
+ |c| TfaUpdateInfo {
+ challenge: Some(c),
+ ..Default::default()
+ },
+ ),
+ Some(challenge) => {
+ let value = value.ok_or_else(|| {
+ format_err!("missing 'value' parameter (u2f challenge response missing)")
+ })?;
+ crate::config::tfa::finish_u2f_registration(&userid, &challenge, &value)
+ .map(TfaUpdateInfo::id)
+ }
+ }
+ }
+ TfaType::Recovery => {
+ if totp.or(value).or(challenge).is_some() {
+ bail!("generating recovery tokens does not allow additional parameters");
+ }
+
+ let recovery = crate::config::tfa::add_recovery(&userid)?;
+
+ Ok(TfaUpdateInfo {
+ id: Some("recovery".to_string()),
+ recovery,
+ ..Default::default()
+ })
+ }
+ }
+}
+
+#[api(
+ protected: true,
+ input: {
+ properties: {
+ userid: { type: Userid },
+ id: {
+ description: "the tfa entry id",
+ },
+ description: {
+ description: "A description to distinguish multiple entries from one another",
+ type: String,
+ max_length: 255,
+ optional: true,
+ },
+ enable: {
+ description: "Whether this entry should currently be enabled or disabled",
+ optional: true,
+ },
+ password: {
+ schema: PASSWORD_SCHEMA,
+ optional: true,
+ },
+ },
+ },
+ access: {
+ permission: &Permission::Or(&[
+ &Permission::Privilege(&["access", "users"], PRIV_PERMISSIONS_MODIFY, false),
+ &Permission::UserParam("userid"),
+ ]),
+ },
+)]
+/// Update user's TFA entry description.
+pub fn update_tfa_entry(
+ userid: Userid,
+ id: String,
+ description: Option<String>,
+ enable: Option<bool>,
+ password: Option<String>,
+ rpcenv: &mut dyn RpcEnvironment,
+) -> Result<(), Error> {
+ tfa_update_auth(rpcenv, &userid, password)?;
+
+ let _lock = crate::config::tfa::write_lock()?;
+
+ let mut data = crate::config::tfa::read()?;
+
+ let mut entry = data
+ .users
+ .get_mut(&userid)
+ .and_then(|user| user.find_entry_mut(&id))
+ .ok_or_else(|| http_err!(NOT_FOUND, "no such entry: {}/{}", userid, id))?;
+
+ if let Some(description) = description {
+ entry.description = description;
+ }
+
+ if let Some(enable) = enable {
+ entry.enable = enable;
+ }
+
+ crate::config::tfa::write(&data)?;
+ Ok(())
+}
+
+pub const ROUTER: Router = Router::new()
+ .get(&API_METHOD_LIST_TFA)
+ .match_all("userid", &USER_ROUTER);
+
+const USER_ROUTER: Router = Router::new()
+ .get(&API_METHOD_LIST_USER_TFA)
+ .post(&API_METHOD_ADD_TFA_ENTRY)
+ .match_all("id", &ITEM_ROUTER);
+
+const ITEM_ROUTER: Router = Router::new()
+ .get(&API_METHOD_GET_TFA)
+ .put(&API_METHOD_UPDATE_TFA_ENTRY)
+ .delete(&API_METHOD_DELETE_TFA);
diff --git a/src/config/tfa.rs b/src/config/tfa.rs
index da3a4f9a..ca39b3c0 100644
--- a/src/config/tfa.rs
+++ b/src/config/tfa.rs
@@ -5,6 +5,7 @@ use std::time::Duration;
use anyhow::{bail, format_err, Error};
use serde::{de::Deserializer, Deserialize, Serialize};
use serde_json::Value;
+use webauthn_rs::Webauthn;
use proxmox::api::api;
use proxmox::sys::error::SysError;
@@ -29,38 +30,98 @@ pub struct U2fConfig {
appid: String,
}
+#[derive(Clone, Deserialize, Serialize)]
+pub struct WebauthnConfig {
+ /// Relying party name. Any text identifier.
+ ///
+ /// Changing this *may* break existing credentials.
+ rp: String,
+
+ /// Site origin. Must be a `https://` URL (or `http://localhost`). Should contain the address
+ /// users type in their browsers to access the web interface.
+ ///
+ /// Changing this *may* break existing credentials.
+ origin: String,
+
+ /// Relying part ID. Must be the domain name without protocol, port or location.
+ ///
+ /// Changing this *will* break existing credentials.
+ id: String,
+}
+
+/// For now we just implement this on the configuration this way.
+///
+/// Note that we may consider changing this so `get_origin` returns the `Host:` header provided by
+/// the connecting client.
+impl webauthn_rs::WebauthnConfig for WebauthnConfig {
+ fn get_relying_party_name(&self) -> String {
+ self.rp.clone()
+ }
+
+ fn get_origin(&self) -> &String {
+ &self.origin
+ }
+
+ fn get_relying_party_id(&self) -> String {
+ self.id.clone()
+ }
+}
+
#[derive(Default, Deserialize, Serialize)]
pub struct TfaConfig {
#[serde(skip_serializing_if = "Option::is_none")]
pub u2f: Option<U2fConfig>,
+
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub webauthn: Option<WebauthnConfig>,
+
#[serde(skip_serializing_if = "TfaUsers::is_empty", default)]
pub users: TfaUsers,
}
/// Heper to get a u2f instance from a u2f config, or `None` if there isn't one configured.
fn get_u2f(u2f: &Option<U2fConfig>) -> Option<u2f::U2f> {
- u2f.as_ref().map(|cfg| u2f::U2f::new(cfg.appid.clone(), cfg.appid.clone()))
+ u2f.as_ref()
+ .map(|cfg| u2f::U2f::new(cfg.appid.clone(), cfg.appid.clone()))
}
/// Heper to get a u2f instance from a u2f config.
-// deduplicate error message while working around self-borrow issue
+///
+/// This is outside of `TfaConfig` to not borrow its `&self`.
fn need_u2f(u2f: &Option<U2fConfig>) -> Result<u2f::U2f, Error> {
get_u2f(u2f).ok_or_else(|| format_err!("no u2f configuration available"))
}
-impl TfaConfig {
- fn u2f(&self) -> Option<u2f::U2f> {
- get_u2f(&self.u2f)
- }
+/// Heper to get a u2f instance from a u2f config, or `None` if there isn't one configured.
+fn get_webauthn(waconfig: &Option<WebauthnConfig>) -> Option<Webauthn<WebauthnConfig>> {
+ waconfig.clone().map(Webauthn::new)
+}
+/// Heper to get a u2f instance from a u2f config.
+///
+/// This is outside of `TfaConfig` to not borrow its `&self`.
+fn need_webauthn(waconfig: &Option<WebauthnConfig>) -> Result<Webauthn<WebauthnConfig>, Error> {
+ get_webauthn(waconfig).ok_or_else(|| format_err!("no webauthn configuration available"))
+}
+
+impl TfaConfig {
+ /// Helper to get a u2f instance. Note that there's a non-&self borrowing version if needed.
fn need_u2f(&self) -> Result<u2f::U2f, Error> {
need_u2f(&self.u2f)
}
+ /// Helper to get a Webauthn instance. Note that there's a non-&self borrowing version if
+ /// needed.
+ fn need_webauthn(&self) -> Result<Webauthn<WebauthnConfig>, Error> {
+ need_webauthn(&self.webauthn)
+ }
+
/// Get a two factor authentication challenge for a user, if the user has TFA set up.
- pub fn login_challenge(&self, userid: &Userid) -> Result<Option<TfaChallenge>, Error> {
- match self.users.get(userid) {
- Some(udata) => udata.challenge(self.u2f().as_ref()),
+ pub fn login_challenge(&mut self, userid: &Userid) -> Result<Option<TfaChallenge>, Error> {
+ match self.users.get_mut(userid) {
+ Some(udata) => {
+ udata.challenge(get_webauthn(&self.webauthn), get_u2f(&self.u2f).as_ref())
+ }
None => Ok(None),
}
}
@@ -94,6 +155,39 @@ impl TfaConfig {
}
}
+ /// Get a webauthn registration challenge.
+ fn webauthn_registration_challenge(
+ &mut self,
+ user: &Userid,
+ description: String,
+ ) -> Result<String, Error> {
+ let webauthn = self.need_webauthn()?;
+
+ self.users
+ .entry(user.clone())
+ .or_default()
+ .webauthn_registration_challenge(webauthn, user, description)
+ }
+
+ /// Finish a webauthn registration challenge.
+ fn webauthn_registration_finish(
+ &mut self,
+ userid: &Userid,
+ challenge: &str,
+ response: &str,
+ ) -> Result<String, Error> {
+ let webauthn = self.need_webauthn()?;
+
+ let response: webauthn_rs::proto::RegisterPublicKeyCredential =
+ serde_json::from_str(response)
+ .map_err(|err| format_err!("error parsing challenge response: {}", err))?;
+
+ match self.users.get_mut(userid) {
+ Some(user) => user.webauthn_registration_finish(webauthn, challenge, response),
+ None => bail!("no such challenge"),
+ }
+ }
+
/// Verify a TFA response.
fn verify(
&mut self,
@@ -102,19 +196,21 @@ impl TfaConfig {
response: TfaResponse,
) -> Result<(), Error> {
match self.users.get_mut(userid) {
- Some(user) => {
- match response {
- TfaResponse::Totp(value) => user.verify_totp(&value),
- TfaResponse::U2f(value) => match &challenge.u2f {
- Some(challenge) => {
- let u2f = need_u2f(&self.u2f)?;
- user.verify_u2f(u2f, &challenge.challenge, value)
- }
- None => bail!("no u2f factor available for user '{}'", userid),
+ Some(user) => match response {
+ TfaResponse::Totp(value) => user.verify_totp(&value),
+ TfaResponse::U2f(value) => match &challenge.u2f {
+ Some(challenge) => {
+ let u2f = need_u2f(&self.u2f)?;
+ user.verify_u2f(u2f, &challenge.challenge, value)
}
- TfaResponse::Recovery(value) => user.verify_recovery(&value),
+ None => bail!("no u2f factor available for user '{}'", userid),
+ },
+ TfaResponse::Webauthn(value) => {
+ let webauthn = need_webauthn(&self.webauthn)?;
+ user.verify_webauthn(webauthn, value)
}
- }
+ TfaResponse::Recovery(value) => user.verify_recovery(&value),
+ },
None => bail!("no 2nd factor available for user '{}'", userid),
}
}
@@ -175,6 +271,10 @@ impl<T> TfaEntry<T> {
}
}
+trait IsExpired {
+ fn is_expired(&self, at_epoch: i64) -> bool;
+}
+
/// A u2f registration challenge.
#[derive(Deserialize, Serialize)]
#[serde(deny_unknown_fields)]
@@ -197,7 +297,79 @@ impl U2fRegistrationChallenge {
created: proxmox::tools::time::epoch_i64(),
}
}
+}
+impl IsExpired for U2fRegistrationChallenge {
+ fn is_expired(&self, at_epoch: i64) -> bool {
+ self.created < at_epoch
+ }
+}
+
+/// A webauthn registration challenge.
+#[derive(Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]
+pub struct WebauthnRegistrationChallenge {
+ /// Server side registration state data.
+ state: webauthn_rs::RegistrationState,
+
+ /// While this is basically the content of a `RegistrationState`, the webauthn-rs crate doesn't
+ /// make this public.
+ challenge: String,
+
+ /// The description chosen by the user for this registration.
+ description: String,
+
+ /// When the challenge was created as unix epoch. They are supposed to be short-lived.
+ created: i64,
+}
+
+impl WebauthnRegistrationChallenge {
+ pub fn new(
+ state: webauthn_rs::RegistrationState,
+ challenge: String,
+ description: String,
+ ) -> Self {
+ Self {
+ state,
+ challenge,
+ description,
+ created: proxmox::tools::time::epoch_i64(),
+ }
+ }
+}
+
+impl IsExpired for WebauthnRegistrationChallenge {
+ fn is_expired(&self, at_epoch: i64) -> bool {
+ self.created < at_epoch
+ }
+}
+
+/// A webauthn authentication challenge.
+#[derive(Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]
+pub struct WebauthnAuthChallenge {
+ /// Server side authentication state.
+ state: webauthn_rs::AuthenticationState,
+
+ /// While this is basically the content of a `AuthenticationState`, the webauthn-rs crate
+ /// doesn't make this public.
+ challenge: String,
+
+ /// When the challenge was created as unix epoch. They are supposed to be short-lived.
+ created: i64,
+}
+
+impl WebauthnAuthChallenge {
+ pub fn new(state: webauthn_rs::AuthenticationState, challenge: String) -> Self {
+ Self {
+ state,
+ challenge,
+ created: proxmox::tools::time::epoch_i64(),
+ }
+ }
+}
+
+impl IsExpired for WebauthnAuthChallenge {
fn is_expired(&self, at_epoch: i64) -> bool {
self.created < at_epoch
}
@@ -216,6 +388,10 @@ pub struct TfaUserData {
#[serde(skip_serializing_if = "Vec::is_empty", default)]
pub(crate) u2f: Vec<TfaEntry<u2f::Registration>>,
+ /// Registered webauthn tokens for a user.
+ #[serde(skip_serializing_if = "Vec::is_empty", default)]
+ pub(crate) webauthn: Vec<TfaEntry<webauthn_rs::proto::Credential>>,
+
/// Recovery keys. (Unordered OTP values).
#[serde(skip_serializing_if = "Vec::is_empty", default)]
pub(crate) recovery: Vec<String>,
@@ -224,23 +400,36 @@ pub struct TfaUserData {
///
/// Expired values are automatically filtered out while parsing the tfa configuration file.
#[serde(skip_serializing_if = "Vec::is_empty", default)]
- #[serde(deserialize_with = "filter_expired_registrations")]
+ #[serde(deserialize_with = "filter_expired_challenge")]
u2f_registrations: Vec<U2fRegistrationChallenge>,
+
+ /// Active webauthn registration challenges for a user.
+ ///
+ /// Expired values are automatically filtered out while parsing the tfa configuration file.
+ #[serde(skip_serializing_if = "Vec::is_empty", default)]
+ #[serde(deserialize_with = "filter_expired_challenge")]
+ webauthn_registrations: Vec<WebauthnRegistrationChallenge>,
+
+ /// Active webauthn registration challenges for a user.
+ ///
+ /// Expired values are automatically filtered out while parsing the tfa configuration file.
+ #[serde(skip_serializing_if = "Vec::is_empty", default)]
+ #[serde(deserialize_with = "filter_expired_challenge")]
+ webauthn_auths: Vec<WebauthnAuthChallenge>,
}
/// Serde helper using our `FilteredVecVisitor` to filter out expired entries directly at load
/// time.
-fn filter_expired_registrations<'de, D>(
- deserializer: D,
-) -> Result<Vec<U2fRegistrationChallenge>, D::Error>
+fn filter_expired_challenge<'de, D, T>(deserializer: D) -> Result<Vec<T>, D::Error>
where
D: Deserializer<'de>,
+ T: Deserialize<'de> + IsExpired,
{
let expire_before = proxmox::tools::time::epoch_i64() - CHALLENGE_TIMEOUT;
Ok(
deserializer.deserialize_seq(crate::tools::serde_filter::FilteredVecVisitor::new(
- "a u2f registration challenge entry",
- move |reg: &U2fRegistrationChallenge| !reg.is_expired(expire_before),
+ "a challenge entry",
+ move |reg: &T| !reg.is_expired(expire_before),
))?,
)
}
@@ -248,7 +437,10 @@ where
impl TfaUserData {
/// `true` if no second factors exist
pub fn is_empty(&self) -> bool {
- self.totp.is_empty() && self.u2f.is_empty() && self.recovery.is_empty()
+ self.totp.is_empty()
+ && self.u2f.is_empty()
+ && self.webauthn.is_empty()
+ && self.recovery.is_empty()
}
/// Find an entry by id, except for the "recovery" entry which we're currently treating
@@ -260,6 +452,12 @@ impl TfaUserData {
}
}
+ for entry in &mut self.webauthn {
+ if entry.info.id == id {
+ return Some(&mut entry.info);
+ }
+ }
+
for entry in &mut self.u2f {
if entry.info.id == id {
return Some(&mut entry.info);
@@ -337,8 +535,71 @@ impl TfaUserData {
Ok(id)
}
+ /// Create a webauthn registration challenge.
+ ///
+ /// The description is required at this point already mostly to better be able to identify such
+ /// challenges in the tfa config file if necessary. The user otherwise has no access to this
+ /// information at this point, as the challenge is identified by its actual challenge data
+ /// instead.
+ fn webauthn_registration_challenge(
+ &mut self,
+ mut webauthn: Webauthn<WebauthnConfig>,
+ userid: &Userid,
+ description: String,
+ ) -> Result<String, Error> {
+ let userid_str = userid.to_string();
+ let (challenge, state) = webauthn.generate_challenge_register(&userid_str, None)?;
+ let challenge_string = challenge.public_key.challenge.to_string();
+ let challenge = serde_json::to_string(&challenge)?;
+
+ self.webauthn_registrations
+ .push(WebauthnRegistrationChallenge::new(
+ state,
+ challenge_string,
+ description,
+ ));
+
+ Ok(challenge)
+ }
+
+ /// Finish a webauthn registration. The challenge should correspond to an output of
+ /// `webauthn_registration_challenge`. The response should come directly from the client.
+ fn webauthn_registration_finish(
+ &mut self,
+ webauthn: Webauthn<WebauthnConfig>,
+ challenge: &str,
+ response: webauthn_rs::proto::RegisterPublicKeyCredential,
+ ) -> Result<String, Error> {
+ let expire_before = proxmox::tools::time::epoch_i64() - CHALLENGE_TIMEOUT;
+
+ let index = self
+ .webauthn_registrations
+ .iter()
+ .position(|r| r.challenge == challenge)
+ .ok_or_else(|| format_err!("no such challenge"))?;
+
+ let reg = self.webauthn_registrations.remove(index);
+ if reg.is_expired(expire_before) {
+ bail!("no such challenge");
+ }
+
+ let credential =
+ webauthn.register_credential(response, reg.state, |id| -> Result<bool, ()> {
+ Ok(self.webauthn.iter().any(|cred| cred.entry.cred_id == *id))
+ })?;
+
+ let entry = TfaEntry::new(reg.description, credential);
+ let id = entry.info.id.clone();
+ self.webauthn.push(entry);
+ Ok(id)
+ }
+
/// Generate a generic TFA challenge. See the [`TfaChallenge`] description for details.
- pub fn challenge(&self, u2f: Option<&u2f::U2f>) -> Result<Option<TfaChallenge>, Error> {
+ pub fn challenge(
+ &mut self,
+ webauthn: Option<Webauthn<WebauthnConfig>>,
+ u2f: Option<&u2f::U2f>,
+ ) -> Result<Option<TfaChallenge>, Error> {
if self.is_empty() {
return Ok(None);
}
@@ -346,6 +607,10 @@ impl TfaUserData {
Ok(Some(TfaChallenge {
totp: self.totp.iter().any(|e| e.info.enable),
recovery: RecoveryState::from_count(self.recovery.len()),
+ webauthn: match webauthn {
+ Some(webauthn) => self.webauthn_challenge(webauthn)?,
+ None => None,
+ },
u2f: match u2f {
Some(u2f) => self.u2f_challenge(u2f)?,
None => None,
@@ -357,26 +622,21 @@ impl TfaUserData {
fn enabled_totp_entries(&self) -> impl Iterator<Item = &Totp> {
self.totp
.iter()
- .filter_map(|e| {
- if e.info.enable {
- Some(&e.entry)
- } else {
- None
- }
- })
+ .filter_map(|e| if e.info.enable { Some(&e.entry) } else { None })
}
/// Helper to iterate over enabled u2f entries.
fn enabled_u2f_entries(&self) -> impl Iterator<Item = &u2f::Registration> {
self.u2f
.iter()
- .filter_map(|e| {
- if e.info.enable {
- Some(&e.entry)
- } else {
- None
- }
- })
+ .filter_map(|e| if e.info.enable { Some(&e.entry) } else { None })
+ }
+
+ /// Helper to iterate over enabled u2f entries.
+ fn enabled_webauthn_entries(&self) -> impl Iterator<Item = &webauthn_rs::proto::Credential> {
+ self.webauthn
+ .iter()
+ .filter_map(|e| if e.info.enable { Some(&e.entry) } else { None })
}
/// Generate an optional u2f challenge.
@@ -400,6 +660,29 @@ impl TfaUserData {
}))
}
+ /// Generate an optional webauthn challenge.
+ fn webauthn_challenge(
+ &mut self,
+ mut webauthn: Webauthn<WebauthnConfig>,
+ ) -> Result<Option<webauthn_rs::proto::RequestChallengeResponse>, Error> {
+ if self.webauthn.is_empty() {
+ return Ok(None);
+ }
+
+ let creds: Vec<_> = self.enabled_webauthn_entries().map(Clone::clone).collect();
+
+ if creds.is_empty() {
+ return Ok(None);
+ }
+
+ let (challenge, state) = webauthn.generate_challenge_authenticate(creds, None)?;
+ let challenge_string = challenge.public_key.challenge.to_string();
+ self.webauthn_auths
+ .push(WebauthnAuthChallenge::new(state, challenge_string));
+
+ Ok(Some(challenge))
+ }
+
/// Verify a totp challenge. The `value` should be the totp digits as plain text.
fn verify_totp(&self, value: &str) -> Result<(), Error> {
let now = std::time::SystemTime::now();
@@ -425,9 +708,12 @@ impl TfaUserData {
if let Some(entry) = self
.enabled_u2f_entries()
- .find(|e| e.key.key_handle == response.key_handle)
+ .find(|e| e.key.key_handle == response.key_handle())
{
- if u2f.auth_verify_obj(&entry.public_key, &challenge.challenge, response)?.is_some() {
+ if u2f
+ .auth_verify_obj(&entry.public_key, &challenge.challenge, response)?
+ .is_some()
+ {
return Ok(());
}
}
@@ -435,6 +721,44 @@ impl TfaUserData {
bail!("u2f verification failed");
}
+ /// Verify a webauthn response.
+ fn verify_webauthn(
+ &mut self,
+ mut webauthn: Webauthn<WebauthnConfig>,
+ mut response: Value,
+ ) -> Result<(), Error> {
+ let expire_before = proxmox::tools::time::epoch_i64() - CHALLENGE_TIMEOUT;
+
+ let challenge = match response
+ .as_object_mut()
+ .ok_or_else(|| format_err!("invalid response, must be a json object"))?
+ .remove("challenge")
+ .ok_or_else(|| format_err!("missing challenge data in response"))?
+ {
+ Value::String(s) => s,
+ _ => bail!("invalid challenge data in response"),
+ };
+
+ let response: webauthn_rs::proto::PublicKeyCredential = serde_json::from_value(response)
+ .map_err(|err| format_err!("invalid webauthn response: {}", err))?;
+
+ let index = self
+ .webauthn_auths
+ .iter()
+ .position(|r| r.challenge == challenge)
+ .ok_or_else(|| format_err!("no such challenge"))?;
+
+ let challenge = self.webauthn_auths.remove(index);
+ if challenge.is_expired(expire_before) {
+ bail!("no such challenge");
+ }
+
+ match webauthn.authenticate_credential(response, challenge.state)? {
+ Some((_cred, _counter)) => Ok(()),
+ None => bail!("webauthn authentication failed"),
+ }
+ }
+
/// Verify a recovery key.
///
/// NOTE: If successful, the key will automatically be removed from the list of available
@@ -458,7 +782,8 @@ impl TfaUserData {
let mut key_data = [0u8; 40]; // 10 keys of 32 bits
proxmox::sys::linux::fill_with_random_data(&mut key_data)?;
for b in key_data.chunks(4) {
- self.recovery.push(format!("{:02x}{:02x}{:02x}{:02x}", b[0], b[1], b[2], b[3]));
+ self.recovery
+ .push(format!("{:02x}{:02x}{:02x}{:02x}", b[0], b[1], b[2], b[3]));
}
Ok(self.recovery.clone())
@@ -493,9 +818,23 @@ pub fn write_lock() -> Result<File, Error> {
proxmox::tools::fs::open_file_locked(LOCK_FILE, LOCK_TIMEOUT, true)
}
+/// Get an optional TFA challenge for a user.
+pub fn login_challenge(userid: &Userid) -> Result<Option<TfaChallenge>, Error> {
+ let _lock = write_lock()?;
+
+ let mut data = read()?;
+ Ok(match data.login_challenge(userid)? {
+ Some(challenge) => {
+ write(&data)?;
+ Some(challenge)
+ }
+ None => None,
+ })
+}
+
/// Add a TOTP entry for a user. Returns the ID.
pub fn add_totp(userid: &Userid, description: String, value: Totp) -> Result<String, Error> {
- let _lock = crate::config::tfa::write_lock();
+ let _lock = write_lock();
let mut data = read()?;
let entry = TfaEntry::new(description, value);
let id = entry.info.id.clone();
@@ -510,10 +849,14 @@ pub fn add_totp(userid: &Userid, description: String, value: Totp) -> Result<Str
/// Add recovery tokens for the user. Returns the token list.
pub fn add_recovery(userid: &Userid) -> Result<Vec<String>, Error> {
- let _lock = crate::config::tfa::write_lock();
+ let _lock = write_lock();
let mut data = read()?;
- let out = data.users.entry(userid.clone()).or_default().add_recovery()?;
+ let out = data
+ .users
+ .entry(userid.clone())
+ .or_default()
+ .add_recovery()?;
write(&data)?;
Ok(out)
}
@@ -535,11 +878,33 @@ pub fn finish_u2f_registration(
) -> Result<String, Error> {
let _lock = crate::config::tfa::write_lock();
let mut data = read()?;
- let challenge = data.u2f_registration_finish(userid, challenge, response)?;
+ let id = data.u2f_registration_finish(userid, challenge, response)?;
+ write(&data)?;
+ Ok(id)
+}
+
+/// Add a webauthn registration challenge for a user.
+pub fn add_webauthn_registration(userid: &Userid, description: String) -> Result<String, Error> {
+ let _lock = crate::config::tfa::write_lock();
+ let mut data = read()?;
+ let challenge = data.webauthn_registration_challenge(userid, description)?;
write(&data)?;
Ok(challenge)
}
+/// Finish a webauthn registration challenge for a user.
+pub fn finish_webauthn_registration(
+ userid: &Userid,
+ challenge: &str,
+ response: &str,
+) -> Result<String, Error> {
+ let _lock = crate::config::tfa::write_lock();
+ let mut data = read()?;
+ let id = data.webauthn_registration_finish(userid, challenge, response)?;
+ write(&data)?;
+ Ok(id)
+}
+
/// Verify a TFA challenge.
pub fn verify_challenge(
userid: &Userid,
@@ -585,7 +950,8 @@ impl Default for RecoveryState {
}
/// When sending a TFA challenge to the user, we include information about what kind of challenge
-/// the user may perform. If u2f devices are available, a u2f challenge will be included.
+/// the user may perform. If webauthn credentials are available, a webauthn challenge will be
+/// included.
#[derive(Deserialize, Serialize)]
#[serde(rename_all = "kebab-case")]
pub struct TfaChallenge {
@@ -599,6 +965,11 @@ pub struct TfaChallenge {
/// If the user has any u2f tokens registered, this will contain the U2F challenge data.
#[serde(skip_serializing_if = "Option::is_none")]
u2f: Option<U2fChallenge>,
+
+ /// If the user has any webauthn credentials registered, this will contain the corresponding
+ /// challenge data.
+ #[serde(skip_serializing_if = "Option::is_none", skip_deserializing)]
+ webauthn: Option<webauthn_rs::proto::RequestChallengeResponse>,
}
/// Data used for u2f challenges.
@@ -615,6 +986,7 @@ pub struct U2fChallenge {
pub enum TfaResponse {
Totp(String),
U2f(Value),
+ Webauthn(Value),
Recovery(String),
}
@@ -626,6 +998,8 @@ impl std::str::FromStr for TfaResponse {
TfaResponse::Totp(s[5..].to_string())
} else if s.starts_with("u2f:") {
TfaResponse::U2f(serde_json::from_str(&s[4..])?)
+ } else if s.starts_with("webauthn:") {
+ TfaResponse::Webauthn(serde_json::from_str(&s[9..])?)
} else if s.starts_with("recovery:") {
TfaResponse::Recovery(s[9..].to_string())
} else {
diff --git a/src/server.rs b/src/server.rs
index 983a300d..7c159c23 100644
--- a/src/server.rs
+++ b/src/server.rs
@@ -87,3 +87,5 @@ pub use email_notifications::*;
mod report;
pub use report::*;
+
+pub mod ticket;
diff --git a/src/server/rest.rs b/src/server/rest.rs
index da110507..7a5bf3ea 100644
--- a/src/server/rest.rs
+++ b/src/server/rest.rs
@@ -599,8 +599,9 @@ fn check_auth(
let ticket = user_auth_data.ticket.clone();
let ticket_lifetime = tools::ticket::TICKET_LIFETIME;
- let userid: Userid = Ticket::parse(&ticket)?
- .verify_with_time_frame(public_auth_key(), "PBS", None, -300..ticket_lifetime)?;
+ let userid: Userid = Ticket::<super::ticket::ApiTicket>::parse(&ticket)?
+ .verify_with_time_frame(public_auth_key(), "PBS", None, -300..ticket_lifetime)?
+ .require_full()?;
let auth_id = Authid::from(userid.clone());
if !user_info.is_active_auth_id(&auth_id) {
diff --git a/src/server/ticket.rs b/src/server/ticket.rs
new file mode 100644
index 00000000..0142a03a
--- /dev/null
+++ b/src/server/ticket.rs
@@ -0,0 +1,77 @@
+use std::fmt;
+
+use anyhow::{bail, Error};
+use serde::{Deserialize, Serialize};
+
+use crate::api2::types::Userid;
+use crate::config::tfa;
+
+#[derive(Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]
+pub struct PartialTicket {
+ #[serde(rename = "u")]
+ userid: Userid,
+
+ #[serde(rename = "c")]
+ challenge: tfa::TfaChallenge,
+}
+
+/// A new ticket struct used in rest.rs's `check_auth` - mostly for better errors than failing to
+/// parse the userid ticket content.
+pub enum ApiTicket {
+ Full(Userid),
+ Partial(tfa::TfaChallenge),
+}
+
+impl ApiTicket {
+ /// Require the ticket to be a full ticket, otherwise error with a meaningful error message.
+ pub fn require_full(self) -> Result<Userid, Error> {
+ match self {
+ ApiTicket::Full(userid) => Ok(userid),
+ ApiTicket::Partial(_) => bail!("access denied - second login factor required"),
+ }
+ }
+
+ /// Expect the ticket to contain a tfa challenge, otherwise error with a meaningful error
+ /// message.
+ pub fn require_partial(self) -> Result<tfa::TfaChallenge, Error> {
+ match self {
+ ApiTicket::Full(_) => bail!("invalid tfa challenge"),
+ ApiTicket::Partial(challenge) => Ok(challenge),
+ }
+ }
+
+ /// Create a new full ticket.
+ pub fn full(userid: Userid) -> Self {
+ ApiTicket::Full(userid)
+ }
+
+ /// Create a new partial ticket.
+ pub fn partial(challenge: tfa::TfaChallenge) -> Self {
+ ApiTicket::Partial(challenge)
+ }
+}
+
+impl fmt::Display for ApiTicket {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ match self {
+ ApiTicket::Full(userid) => fmt::Display::fmt(userid, f),
+ ApiTicket::Partial(partial) => {
+ let data = serde_json::to_string(partial).map_err(|_| fmt::Error)?;
+ write!(f, "!tfa!{}", data)
+ }
+ }
+ }
+}
+
+impl std::str::FromStr for ApiTicket {
+ type Err = Error;
+
+ fn from_str(s: &str) -> Result<Self, Error> {
+ if s.starts_with("!tfa!") {
+ Ok(ApiTicket::Partial(serde_json::from_str(&s[5..])?))
+ } else {
+ Ok(ApiTicket::Full(s.parse()?))
+ }
+ }
+}
--
2.20.1
^ permalink raw reply [flat|nested] 23+ messages in thread* [pbs-devel] [RFC backup 4/6] depend on libjs-qrcodejs
2020-11-19 14:56 [pbs-devel] [RFC backup 0/6] Two factor authentication Wolfgang Bumiller
` (2 preceding siblings ...)
2020-11-19 14:56 ` [pbs-devel] [RFC backup 3/6] api: tfa management and login Wolfgang Bumiller
@ 2020-11-19 14:56 ` Wolfgang Bumiller
2020-11-19 14:56 ` [pbs-devel] [RFC backup 5/6] proxy: expose qrcodejs Wolfgang Bumiller
` (2 subsequent siblings)
6 siblings, 0 replies; 23+ messages in thread
From: Wolfgang Bumiller @ 2020-11-19 14:56 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
debian/control.in | 1 +
1 file changed, 1 insertion(+)
diff --git a/debian/control.in b/debian/control.in
index 75987dbb..899b9763 100644
--- a/debian/control.in
+++ b/debian/control.in
@@ -2,6 +2,7 @@ Package: proxmox-backup-server
Architecture: any
Depends: fonts-font-awesome,
libjs-extjs (>= 6.0.1),
+ libjs-qrcodejs (>= 1.20201119),
libzstd1 (>= 1.3.8),
lvm2,
openssh-server,
--
2.20.1
^ permalink raw reply [flat|nested] 23+ messages in thread* [pbs-devel] [RFC backup 5/6] proxy: expose qrcodejs
2020-11-19 14:56 [pbs-devel] [RFC backup 0/6] Two factor authentication Wolfgang Bumiller
` (3 preceding siblings ...)
2020-11-19 14:56 ` [pbs-devel] [RFC backup 4/6] depend on libjs-qrcodejs Wolfgang Bumiller
@ 2020-11-19 14:56 ` Wolfgang Bumiller
2020-11-19 14:56 ` [pbs-devel] [RFC backup 6/6] gui: tfa support Wolfgang Bumiller
2020-12-02 10:56 ` [pbs-devel] [RFC backup 0/6] Two factor authentication Oguz Bektas
6 siblings, 0 replies; 23+ messages in thread
From: Wolfgang Bumiller @ 2020-11-19 14:56 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
src/bin/proxmox-backup-proxy.rs | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index 259d558a..6414d646 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -86,6 +86,7 @@ async fn run() -> Result<(), Error> {
config.add_alias("novnc", "/usr/share/novnc-pve");
config.add_alias("extjs", "/usr/share/javascript/extjs");
+ config.add_alias("qrcodejs", "/usr/share/javascript/qrcodejs");
config.add_alias("fontawesome", "/usr/share/fonts-font-awesome");
config.add_alias("xtermjs", "/usr/share/pve-xtermjs");
config.add_alias("locale", "/usr/share/pbs-i18n");
--
2.20.1
^ permalink raw reply [flat|nested] 23+ messages in thread* [pbs-devel] [RFC backup 6/6] gui: tfa support
2020-11-19 14:56 [pbs-devel] [RFC backup 0/6] Two factor authentication Wolfgang Bumiller
` (4 preceding siblings ...)
2020-11-19 14:56 ` [pbs-devel] [RFC backup 5/6] proxy: expose qrcodejs Wolfgang Bumiller
@ 2020-11-19 14:56 ` Wolfgang Bumiller
2020-11-24 9:42 ` Wolfgang Bumiller
2020-12-02 10:56 ` [pbs-devel] [RFC backup 0/6] Two factor authentication Oguz Bektas
6 siblings, 1 reply; 23+ messages in thread
From: Wolfgang Bumiller @ 2020-11-19 14:56 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
www/LoginView.js | 323 +++++++++++++++++++++++++++++++++--
www/Makefile | 6 +
www/OnlineHelpInfo.js | 36 ----
www/Utils.js | 59 +++++++
www/config/TfaView.js | 322 ++++++++++++++++++++++++++++++++++
www/index.hbs | 1 +
www/panel/AccessControl.js | 6 +
www/window/AddTfaRecovery.js | 211 +++++++++++++++++++++++
www/window/AddTotp.js | 283 ++++++++++++++++++++++++++++++
www/window/AddWebauthn.js | 193 +++++++++++++++++++++
www/window/TfaEdit.js | 92 ++++++++++
11 files changed, 1478 insertions(+), 54 deletions(-)
create mode 100644 www/config/TfaView.js
create mode 100644 www/window/AddTfaRecovery.js
create mode 100644 www/window/AddTotp.js
create mode 100644 www/window/AddWebauthn.js
create mode 100644 www/window/TfaEdit.js
diff --git a/www/LoginView.js b/www/LoginView.js
index 1deba415..305f4c0d 100644
--- a/www/LoginView.js
+++ b/www/LoginView.js
@@ -5,7 +5,7 @@ Ext.define('PBS.LoginView', {
controller: {
xclass: 'Ext.app.ViewController',
- submitForm: function() {
+ submitForm: async function() {
var me = this;
var loginForm = me.lookupReference('loginForm');
var unField = me.lookupReference('usernameField');
@@ -33,24 +33,51 @@ Ext.define('PBS.LoginView', {
}
sp.set(saveunField.getStateId(), saveunField.getValue());
- Proxmox.Utils.API2Request({
- url: '/api2/extjs/access/ticket',
- params: params,
- method: 'POST',
- success: function(resp, opts) {
- // save login data and create cookie
- PBS.Utils.updateLoginData(resp.result.data);
- PBS.app.changeView('mainview');
- },
- failure: function(resp, opts) {
- Proxmox.Utils.authClear();
- loginForm.unmask();
- Ext.MessageBox.alert(
- gettext('Error'),
- gettext('Login failed. Please try again'),
- );
- },
+ try {
+ let resp = await PBS.Async.api2({
+ url: '/api2/extjs/access/ticket',
+ params: params,
+ method: 'POST',
+ });
+
+ let data = resp.result.data;
+ if (data.ticket.startsWith("PBS:!tfa!")) {
+ data = await me.performTFAChallenge(data);
+ }
+
+ PBS.Utils.updateLoginData(data);
+ PBS.app.changeView('mainview');
+ } catch (error) {
+ console.error(error); // for debugging
+ Proxmox.Utils.authClear();
+ loginForm.unmask();
+ Ext.MessageBox.alert(
+ gettext('Error'),
+ gettext('Login failed. Please try again'),
+ );
+ }
+ },
+
+ performTFAChallenge: async function(data) {
+ let me = this;
+
+ let userid = data.username;
+ let ticket = data.ticket;
+ let challenge = JSON.parse(decodeURIComponent(
+ ticket.split(':')[1].slice("!tfa!".length),
+ ));
+
+ let resp = await new Promise((resolve, reject) => {
+ Ext.create('PBS.login.TfaWindow', {
+ userid,
+ ticket,
+ challenge,
+ onResolve: value => resolve(value),
+ onReject: reject,
+ }).show();
});
+
+ return resp.result.data;
},
control: {
@@ -209,3 +236,263 @@ Ext.define('PBS.LoginView', {
},
],
});
+
+Ext.define('PBS.login.TfaWindow', {
+ extend: 'Ext.window.Window',
+ mixins: ['Proxmox.Mixin.CBind'],
+
+ modal: true,
+ resizable: false,
+ title: gettext("Second login factor required"),
+
+ cancelled: true,
+
+ width: 512,
+ layout: {
+ type: 'vbox',
+ align: 'stretch',
+ },
+
+ defaultButton: 'totpButton',
+
+ viewModel: {
+ data: {
+ userid: undefined,
+ ticket: undefined,
+ challenge: undefined,
+ },
+ },
+
+ controller: {
+ xclass: 'Ext.app.ViewController',
+
+ init: function(view) {
+ let me = this;
+
+ if (!view.userid) {
+ throw "no userid given";
+ }
+
+ if (!view.ticket) {
+ throw "no ticket given";
+ }
+
+ if (!view.challenge) {
+ throw "no challenge given";
+ }
+
+ if (!view.challenge.webauthn) {
+ me.lookup('webauthnButton').setVisible(false);
+ }
+
+ if (!view.challenge.totp) {
+ me.lookup('totpButton').setVisible(false);
+ }
+
+ if (!view.challenge.recovery) {
+ me.lookup('recoveryButton').setVisible(false);
+ } else if (view.challenge.recovery === "low") {
+ me.lookup('recoveryButton')
+ .setIconCls('fa fa-fw fa-exclamation-triangle');
+ }
+
+
+ if (!view.challenge.totp && !view.challenge.recovery) {
+ // only webauthn tokens available, maybe skip ahead?
+ me.lookup('totp').setVisible(false);
+ me.lookup('waiting').setVisible(true);
+ let _promise = me.loginWebauthn();
+ }
+ },
+
+ onClose: function() {
+ let me = this;
+ let view = me.getView();
+
+ if (!view.cancelled) {
+ return;
+ }
+
+ view.onReject();
+ },
+
+ cancel: function() {
+ this.getView().close();
+ },
+
+ loginTotp: function() {
+ let me = this;
+
+ let _promise = me.finishChallenge('totp:' + me.lookup('totp').value);
+ },
+
+ loginWebauthn: async function() {
+ let me = this;
+ let view = me.getView();
+
+ // avoid this window ending up above the tfa popup if we got triggered from init().
+ await PBS.Async.sleep(100);
+
+ // FIXME: With webauthn the browser provides a popup (since it doesn't necessarily need
+ // to require pressing a button, but eg. use a fingerprint scanner or face detection
+ // etc., so should we just trust that that happens and skip the popup?)
+ let msg = Ext.Msg.show({
+ title: `Webauthn: ${gettext('Login')}`,
+ message: gettext('Please press the button on your Authenticator Device'),
+ buttons: [],
+ });
+
+ let challenge = view.challenge.webauthn;
+
+ // Byte array fixup, keep challenge string:
+ let challenge_str = challenge.publicKey.challenge;
+ challenge.publicKey.challenge = PBS.Utils.base64url_to_bytes(challenge_str);
+ for (const cred of challenge.publicKey.allowCredentials) {
+ cred.id = PBS.Utils.base64url_to_bytes(cred.id);
+ }
+
+ let hwrsp;
+ try {
+ hwrsp = await navigator.credentials.get(challenge);
+ } catch (error) {
+ view.onReject(error);
+ return;
+ } finally {
+ msg.close();
+ }
+
+ let response = {
+ id: hwrsp.id,
+ type: hwrsp.type,
+ challenge: challenge_str,
+ rawId: PBS.Utils.bytes_to_base64url(hwrsp.rawId),
+ response: {
+ authenticatorData: PBS.Utils.bytes_to_base64url(
+ hwrsp.response.authenticatorData,
+ ),
+ clientDataJSON: PBS.Utils.bytes_to_base64url(hwrsp.response.clientDataJSON),
+ signature: PBS.Utils.bytes_to_base64url(hwrsp.response.signature),
+ },
+ };
+
+ msg.close();
+
+ await me.finishChallenge("webauthn:" + JSON.stringify(response));
+ },
+
+ loginRecovery: function() {
+ let me = this;
+ let view = me.getView();
+
+ if (me.login_recovery_confirm) {
+ let _promise = me.finishChallenge('recovery:' + me.lookup('totp').value);
+ } else {
+ me.login_recovery_confirm = true;
+ me.lookup('totpButton').setVisible(false);
+ me.lookup('webauthnButton').setVisible(false);
+ me.lookup('recoveryButton').setText(gettext("Confirm"));
+ me.lookup('recoveryInfo').setVisible(true);
+ if (view.challenge.recovery === "low") {
+ me.lookup('recoveryLow').setVisible(true);
+ }
+ }
+ },
+
+ finishChallenge: function(password) {
+ let me = this;
+ let view = me.getView();
+ view.cancelled = false;
+
+ let params = {
+ username: view.userid,
+ 'tfa-challenge': view.ticket,
+ password,
+ };
+
+ let resolve = view.onResolve;
+ let reject = view.onReject;
+ view.close();
+
+ return PBS.Async.api2({
+ url: '/api2/extjs/access/ticket',
+ method: 'POST',
+ params,
+ })
+ .then(resolve)
+ .catch(reject);
+ },
+ },
+
+ listeners: {
+ close: 'onClose',
+ },
+
+ items: [
+ {
+ xtype: 'form',
+ layout: 'anchor',
+ border: false,
+ fieldDefaults: {
+ anchor: '100%',
+ padding: '0 5',
+ },
+ items: [
+ {
+ xtype: 'textfield',
+ fieldLabel: gettext('Please enter your OTP verification code:'),
+ labelWidth: '300px',
+ name: 'totp',
+ reference: 'totp',
+ allowBlank: false,
+ },
+ ],
+ },
+ {
+ xtype: 'box',
+ html: gettext('Waiting for second factor.'),
+ reference: 'waiting',
+ padding: '0 5',
+ hidden: true,
+ },
+ {
+ xtype: 'box',
+ padding: '0 5',
+ reference: 'recoveryInfo',
+ hidden: true,
+ html: gettext('Please note that each recovery code can only be used once!'),
+ style: {
+ textAlign: "center",
+ },
+ },
+ {
+ xtype: 'box',
+ padding: '0 5',
+ reference: 'recoveryLow',
+ hidden: true,
+ html: '<i class="fa fa-exclamation-triangle warning"></i>'
+ + gettext('Only few recovery keys available. Please generate a new set!')
+ + '<i class="fa fa-exclamation-triangle warning"></i>',
+ style: {
+ textAlign: "center",
+ },
+ },
+ ],
+
+ buttons: [
+ {
+ text: gettext('Login with TOTP'),
+ handler: 'loginTotp',
+ reference: 'totpButton',
+ },
+ {
+ text: gettext('Login with a recovery key'),
+ handler: 'loginRecovery',
+ reference: 'recoveryButton',
+ },
+ {
+ text: gettext('Use a Webauthn token'),
+ handler: 'loginWebauthn',
+ reference: 'webauthnButton',
+ },
+ ],
+});
diff --git a/www/Makefile b/www/Makefile
index 1df2195a..86e0516e 100644
--- a/www/Makefile
+++ b/www/Makefile
@@ -16,12 +16,16 @@ JSSRC= \
data/RunningTasksStore.js \
button/TaskButton.js \
config/UserView.js \
+ config/TfaView.js \
config/TokenView.js \
config/RemoteView.js \
config/ACLView.js \
config/SyncView.js \
config/VerifyView.js \
window/ACLEdit.js \
+ window/AddTfaRecovery.js \
+ window/AddTotp.js \
+ window/AddWebauthn.js \
window/BackupFileDownloader.js \
window/BackupGroupChangeOwner.js \
window/CreateDirectory.js \
@@ -34,6 +38,7 @@ JSSRC= \
window/UserEdit.js \
window/UserPassword.js \
window/TokenEdit.js \
+ window/TfaEdit.js \
window/VerifyJobEdit.js \
window/ZFSCreate.js \
dashboard/DataStoreStatistics.js \
@@ -100,6 +105,7 @@ install: js/proxmox-backup-gui.js css/ext6-pbs.css index.hbs
install -m644 index.hbs $(DESTDIR)$(JSDIR)/
install -dm755 $(DESTDIR)$(JSDIR)/js
install -m644 js/proxmox-backup-gui.js $(DESTDIR)$(JSDIR)/js/
+ install -m 0644 qrcode.min.js $(DESTDIR)$(JSDIR)/
install -dm755 $(DESTDIR)$(JSDIR)/css
install -m644 css/ext6-pbs.css $(DESTDIR)$(JSDIR)/css/
install -dm755 $(DESTDIR)$(JSDIR)/images
diff --git a/www/OnlineHelpInfo.js b/www/OnlineHelpInfo.js
index aee73bf6..c54912d8 100644
--- a/www/OnlineHelpInfo.js
+++ b/www/OnlineHelpInfo.js
@@ -75,42 +75,6 @@ const proxmoxOnlineHelpInfo = {
"link": "/docs/pve-integration.html#pve-integration",
"title": "`Proxmox VE`_ Integration"
},
- "rst-primer": {
- "link": "/docs/reStructuredText-primer.html#rst-primer",
- "title": "reStructuredText Primer"
- },
- "rst-inline-markup": {
- "link": "/docs/reStructuredText-primer.html#rst-inline-markup",
- "title": "Inline markup"
- },
- "rst-literal-blocks": {
- "link": "/docs/reStructuredText-primer.html#rst-literal-blocks",
- "title": "Literal blocks"
- },
- "rst-doctest-blocks": {
- "link": "/docs/reStructuredText-primer.html#rst-doctest-blocks",
- "title": "Doctest blocks"
- },
- "rst-tables": {
- "link": "/docs/reStructuredText-primer.html#rst-tables",
- "title": "Tables"
- },
- "rst-field-lists": {
- "link": "/docs/reStructuredText-primer.html#rst-field-lists",
- "title": "Field Lists"
- },
- "rst-roles-alt": {
- "link": "/docs/reStructuredText-primer.html#rst-roles-alt",
- "title": "Roles"
- },
- "rst-directives": {
- "link": "/docs/reStructuredText-primer.html#rst-directives",
- "title": "Directives"
- },
- "html-meta": {
- "link": "/docs/reStructuredText-primer.html#html-meta",
- "title": "HTML Metadata"
- },
"storage-disk-management": {
"link": "/docs/storage.html#storage-disk-management",
"title": "Disk Management"
diff --git a/www/Utils.js b/www/Utils.js
index ab48bdcf..1d2810bf 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -284,4 +284,63 @@ Ext.define('PBS.Utils', {
zfscreate: [gettext('ZFS Storage'), gettext('Create')],
});
},
+
+ // Convert an ArrayBuffer to a base64url encoded string.
+ // A `null` value will be preserved for convenience.
+ bytes_to_base64url: function(bytes) {
+ if (bytes === null) {
+ return null;
+ }
+
+ return btoa(Array
+ .from(new Uint8Array(bytes))
+ .map(val => String.fromCharCode(val))
+ .join(''),
+ )
+ .replace(/\+/g, '-')
+ .replace(/\//g, '_')
+ .replace(/[=]/g, '');
+ },
+
+ // Convert an a base64url string to an ArrayBuffer.
+ // A `null` value will be preserved for convenience.
+ base64url_to_bytes: function(b64u) {
+ if (b64u === null) {
+ return null;
+ }
+
+ return new Uint8Array(
+ atob(b64u
+ .replace(/-/g, '+')
+ .replace(/_/g, '/'),
+ )
+ .split('')
+ .map(val => val.charCodeAt(0)),
+ );
+ },
+});
+
+Ext.define('PBS.Async', {
+ singleton: true,
+
+ // Returns a Promise resolving to the result of an `API2Request`.
+ api2: function(reqOpts) {
+ return new Promise((resolve, reject) => {
+ delete reqOpts.callback; // not allowed in this api
+ reqOpts.success = response => resolve(response);
+ reqOpts.failure = response => {
+ if (response.result && response.result.message) {
+ reject(response.result.message);
+ } else {
+ reject("api call failed");
+ }
+ };
+ Proxmox.Utils.API2Request(reqOpts);
+ });
+ },
+
+ // Delay for a number of milliseconds.
+ sleep: function(millis) {
+ return new Promise((resolve, _reject) => setTimeout(resolve, millis));
+ },
});
diff --git a/www/config/TfaView.js b/www/config/TfaView.js
new file mode 100644
index 00000000..350c98a7
--- /dev/null
+++ b/www/config/TfaView.js
@@ -0,0 +1,322 @@
+Ext.define('pbs-tfa-users', {
+ extend: 'Ext.data.Model',
+ fields: ['userid'],
+ idProperty: 'userid',
+ proxy: {
+ type: 'proxmox',
+ url: '/api2/json/access/tfa',
+ },
+});
+
+Ext.define('pbs-tfa-entry', {
+ extend: 'Ext.data.Model',
+ fields: ['fullid', 'type', 'description', 'enable'],
+ idProperty: 'fullid',
+});
+
+
+Ext.define('PBS.config.TfaView', {
+ extend: 'Ext.grid.GridPanel',
+ alias: 'widget.pbsTfaView',
+
+ title: gettext('Second Factors'),
+ reference: 'tfaview',
+
+ store: {
+ type: 'diff',
+ autoDestroy: true,
+ autoDestroyRstore: true,
+ model: 'pbs-tfa-entry',
+ rstore: {
+ type: 'store',
+ proxy: 'memory',
+ storeid: 'pbs-tfa-entry',
+ model: 'pbs-tfa-entry',
+ },
+ },
+
+ controller: {
+ xclass: 'Ext.app.ViewController',
+
+ init: function(view) {
+ let me = this;
+ view.tfaStore = Ext.create('Proxmox.data.UpdateStore', {
+ autoStart: true,
+ interval: 5 * 1000,
+ storeid: 'pbs-tfa-users',
+ model: 'pbs-tfa-users',
+ });
+ view.tfaStore.on('load', this.onLoad, this);
+ view.on('destroy', view.tfaStore.stopUpdate);
+ Proxmox.Utils.monStoreErrors(view, view.tfaStore);
+ },
+
+ reload: function() { this.getView().tfaStore.load(); },
+
+ onLoad: function(store, data, success) {
+ if (!success) return;
+
+ let records = [];
+ Ext.Array.each(data, user => {
+ Ext.Array.each(user.data.entries, entry => {
+ records.push({
+ fullid: `${user.id}/${entry.id}`,
+ type: entry.type,
+ description: entry.description,
+ enable: entry.enable,
+ });
+ });
+ });
+
+ let rstore = this.getView().store.rstore;
+ rstore.loadData(records);
+ rstore.fireEvent('load', rstore, records, true);
+ },
+
+ addTotp: function() {
+ let me = this;
+
+ Ext.create('PBS.window.AddTotp', {
+ isCreate: true,
+ listeners: {
+ destroy: function() {
+ me.reload();
+ },
+ },
+ }).show();
+ },
+
+ addWebauthn: function() {
+ let me = this;
+
+ Ext.create('PBS.window.AddWebauthn', {
+ isCreate: true,
+ listeners: {
+ destroy: function() {
+ me.reload();
+ },
+ },
+ }).show();
+ },
+
+ addRecovery: async function() {
+ let me = this;
+
+ Ext.create('PBS.window.AddTfaRecovery', {
+ listeners: {
+ destroy: function() {
+ me.reload();
+ },
+ },
+ }).show();
+ },
+
+ editItem: function() {
+ let me = this;
+ let view = me.getView();
+ let selection = view.getSelection();
+ if (selection.length !== 1 || selection[0].id.endsWith("/recovery")) {
+ return;
+ }
+
+ Ext.create('PBS.window.TfaEdit', {
+ 'tfa-id': selection[0].data.fullid,
+ listeners: {
+ destroy: function() {
+ me.reload();
+ },
+ },
+ }).show();
+ },
+
+ renderUser: fullid => fullid.split('/')[0],
+
+ renderEnabled: enabled => {
+ if (enabled === undefined) {
+ return Proxmox.Utils.yesText;
+ } else {
+ return Proxmox.Utils.format_boolean(enabled);
+ }
+ },
+
+ onRemoveButton: function(btn, event, record) {
+ let me = this;
+
+ Ext.create('PBS.tfa.confirmRemove', {
+ message: Ext.String.format(
+ gettext('Are you sure you want to remove entry {0}'),
+ record.data.description,
+ ),
+ callback: password => me.removeItem(password, record),
+ })
+ .show();
+ },
+
+ removeItem: async function(password, record) {
+ let me = this;
+
+ let params = {};
+ if (password !== null) {
+ params.password = password;
+ }
+
+ try {
+ await PBS.Async.api2({
+ url: `/api2/extjs/access/tfa/${record.id}`,
+ method: 'DELETE',
+ params,
+ });
+ me.reload();
+ } catch (error) {
+ Ext.Msg.alert(gettext('Error'), error);
+ }
+ },
+ },
+
+ viewConfig: {
+ trackOver: false,
+ },
+
+ listeners: {
+ itemdblclick: 'editItem',
+ },
+
+ columns: [
+ {
+ header: gettext('User'),
+ width: 200,
+ sortable: true,
+ dataIndex: 'fullid',
+ renderer: 'renderUser',
+ },
+ {
+ header: gettext('Enabled'),
+ width: 80,
+ sortable: true,
+ dataIndex: 'enable',
+ renderer: 'renderEnabled',
+ },
+ {
+ header: gettext('TFA Type'),
+ width: 80,
+ sortable: true,
+ dataIndex: 'type',
+ },
+ {
+ header: gettext('Description'),
+ width: 300,
+ sortable: true,
+ dataIndex: 'description',
+ },
+ ],
+
+ tbar: [
+ {
+ text: gettext('Add'),
+ menu: {
+ xtype: 'menu',
+ items: [
+ {
+ text: gettext('TOTP'),
+ itemId: 'totp',
+ iconCls: 'fa fa-fw fa-clock-o',
+ handler: 'addTotp',
+ },
+ {
+ text: gettext('Webauthn'),
+ itemId: 'webauthn',
+ iconCls: 'fa fa-fw fa-shield',
+ handler: 'addWebauthn',
+ },
+ {
+ text: gettext('Recovery Keys'),
+ itemId: 'recovery',
+ iconCls: 'fa fa-fw fa-file-text-o',
+ handler: 'addRecovery',
+ },
+ ],
+ },
+ },
+ {
+ xtype: 'proxmoxButton',
+ text: gettext('Edit'),
+ handler: 'editItem',
+ enableFn: rec => !rec.id.endsWith("/recovery"),
+ disabled: true,
+ },
+ {
+ xtype: 'proxmoxButton',
+ text: gettext('Remove'),
+ getRecordName: rec => rec.data.description,
+ handler: 'onRemoveButton',
+ },
+ ],
+});
+
+Ext.define('PBS.tfa.confirmRemove', {
+ extend: 'Proxmox.window.Edit',
+
+ modal: true,
+ resizable: false,
+ title: gettext("Confirm Password"),
+ width: 512,
+ isCreate: true, // logic
+ isRemove: true,
+
+ url: '/access/tfa',
+
+ initComponent: function() {
+ let me = this;
+
+ if (!me.message) {
+ throw "missing message";
+ }
+
+ if (!me.callback) {
+ throw "missing callback";
+ }
+
+ me.callParent();
+
+ if (Proxmox.UserName === 'root@pam') {
+ me.lookup('password').setVisible(false);
+ me.lookup('password').setDisabled(true);
+ }
+
+ me.lookup('message').setHtml(Ext.String.htmlEncode(me.message));
+ },
+
+ submit: function() {
+ let me = this;
+ if (Proxmox.UserName === 'root@pam') {
+ me.callback(null);
+ } else {
+ me.callback(me.lookup('password').getValue());
+ }
+ me.close();
+ },
+
+ items: [
+ {
+ xtype: 'box',
+ padding: '5 5',
+ reference: 'message',
+ html: gettext(''),
+ style: {
+ textAlign: "center",
+ },
+ },
+ {
+ xtype: 'textfield',
+ inputType: 'password',
+ fieldLabel: gettext('Password'),
+ minLength: 5,
+ reference: 'password',
+ name: 'password',
+ allowBlank: false,
+ validateBlank: true,
+ padding: '0 0 5 5',
+ emptyText: gettext('verify current password'),
+ },
+ ],
+});
diff --git a/www/index.hbs b/www/index.hbs
index 008e2410..665bef23 100644
--- a/www/index.hbs
+++ b/www/index.hbs
@@ -37,6 +37,7 @@
<script type="text/javascript">
Ext.History.fieldid = 'x-history-field';
</script>
+ <script type="text/javascript" src="/qrcodejs/qrcode.min.js"></script>
<script type="text/javascript" src="/js/proxmox-backup-gui.js"></script>
</head>
<body>
diff --git a/www/panel/AccessControl.js b/www/panel/AccessControl.js
index dfb63b60..94690cfe 100644
--- a/www/panel/AccessControl.js
+++ b/www/panel/AccessControl.js
@@ -19,6 +19,12 @@ Ext.define('PBS.AccessControlPanel', {
itemId: 'users',
iconCls: 'fa fa-user',
},
+ {
+ xtype: 'pbsTfaView',
+ title: gettext('Two Factor Authentication'),
+ itemId: 'tfa',
+ iconCls: 'fa fa-key',
+ },
{
xtype: 'pbsTokenView',
title: gettext('API Token'),
diff --git a/www/window/AddTfaRecovery.js b/www/window/AddTfaRecovery.js
new file mode 100644
index 00000000..df94884b
--- /dev/null
+++ b/www/window/AddTfaRecovery.js
@@ -0,0 +1,211 @@
+Ext.define('PBS.window.AddTfaRecovery', {
+ extend: 'Ext.window.Window',
+ alias: 'widget.pbsAddTfaRecovery',
+ mixins: ['Proxmox.Mixin.CBind'],
+
+ onlineHelp: 'user_mgmt',
+
+ modal: true,
+ resizable: false,
+ title: gettext('Add TFA recovery keys'),
+ width: 512,
+
+ fixedUser: false,
+
+ baseurl: '/api2/extjs/access/tfa',
+
+ initComponent: function() {
+ let me = this;
+ me.callParent();
+ Ext.GlobalEvents.fireEvent('proxmoxShowHelp', me.onlineHelp);
+ },
+
+ viewModel: {
+ data: {
+ has_entry: false,
+ },
+ },
+
+ controller: {
+ xclass: 'Ext.app.ViewController',
+ control: {
+ '#': {
+ show: function() {
+ let me = this;
+ let view = me.getView();
+
+ if (Proxmox.UserName === 'root@pam') {
+ view.lookup('password').setVisible(false);
+ view.lookup('password').setDisabled(true);
+ }
+ },
+ },
+ },
+
+ hasEntry: async function(userid) {
+ let me = this;
+ let view = me.getView();
+
+ try {
+ await PBS.Async.api2({
+ url: `${view.baseurl}/${userid}/recovery`,
+ method: 'GET',
+ });
+ return true;
+ } catch (_ex) {
+ return false;
+ }
+ },
+
+ init: function() {
+ this.onUseridChange(null, Proxmox.UserName);
+ },
+
+ onUseridChange: async function(_field, userid) {
+ let me = this;
+
+ me.userid = userid;
+
+ let has_entry = await me.hasEntry(userid);
+ me.getViewModel().set('has_entry', has_entry);
+ },
+
+ onAdd: async function() {
+ let me = this;
+ let view = me.getView();
+
+ let baseurl = view.baseurl;
+
+ let userid = me.userid;
+ if (userid === undefined) {
+ throw "no userid set";
+ }
+
+ me.getView().close();
+
+ try {
+ let response = await PBS.Async.api2({
+ url: `${baseurl}/${userid}`,
+ method: 'POST',
+ params: { type: 'recovery' },
+ });
+ let values = response.result.data.recovery.join("\n");
+ Ext.create('PBS.window.TfaRecoveryShow', {
+ autoShow: true,
+ values,
+ });
+ } catch (ex) {
+ Ext.Msg.alert(gettext('Error'), ex);
+ }
+ },
+ },
+
+ items: [
+ {
+ xtype: 'pmxDisplayEditField',
+ name: 'userid',
+ cbind: {
+ editable: (get) => !get('fixedUser'),
+ },
+ fieldLabel: gettext('User'),
+ editConfig: {
+ xtype: 'pbsUserSelector',
+ allowBlank: false,
+ },
+ renderer: Ext.String.htmlEncode,
+ value: Proxmox.UserName,
+ listeners: {
+ change: 'onUseridChange',
+ },
+ },
+ {
+ xtype: 'displayfield',
+ bind: {
+ hidden: '{!has_entry}',
+ },
+ value: gettext('User already has recovery keys.'),
+ },
+ {
+ xtype: 'textfield',
+ inputType: 'password',
+ fieldLabel: gettext('Password'),
+ minLength: 5,
+ reference: 'password',
+ name: 'password',
+ allowBlank: false,
+ validateBlank: true,
+ padding: '0 0 5 5',
+ emptyText: gettext('verify current password'),
+ },
+ ],
+
+ buttons: [
+ {
+ xtype: 'proxmoxHelpButton',
+ },
+ '->',
+ {
+ xtype: 'button',
+ text: gettext('Add'),
+ handler: 'onAdd',
+ bind: {
+ disabled: '{has_entry}',
+ },
+ },
+ ],
+});
+
+Ext.define('PBS.window.TfaRecoveryShow', {
+ extend: 'Ext.window.Window',
+ alias: ['widget.pbsTfaRecoveryShow'],
+ mixins: ['Proxmox.Mixin.CBind'],
+
+ width: 600,
+ modal: true,
+ resizable: false,
+ title: gettext('Recovery Keys'),
+
+ items: [
+ {
+ xtype: 'container',
+ layout: 'form',
+ bodyPadding: 10,
+ border: false,
+ fieldDefaults: {
+ labelWidth: 100,
+ anchor: '100%',
+ },
+ padding: '0 10 10 10',
+ items: [
+ {
+ xtype: 'textarea',
+ editable: false,
+ inputId: 'token-secret-value',
+ cbind: {
+ value: '{values}',
+ },
+ fieldStyle: {
+ 'fontFamily': 'monospace',
+ },
+ height: '160px',
+ },
+ ],
+ },
+ {
+ xtype: 'component',
+ border: false,
+ padding: '10 10 10 10',
+ userCls: 'pmx-hint',
+ html: gettext('Please record recovery keys - they will only be displayed now'),
+ },
+ ],
+ buttons: [
+ {
+ handler: function(b) {
+ document.getElementById('token-secret-value').select();
+ document.execCommand("copy");
+ },
+ text: gettext('Copy Secret Value'),
+ },
+ ],
+});
diff --git a/www/window/AddTotp.js b/www/window/AddTotp.js
new file mode 100644
index 00000000..40417340
--- /dev/null
+++ b/www/window/AddTotp.js
@@ -0,0 +1,283 @@
+/*global QRCode*/
+Ext.define('PBS.window.AddTotp', {
+ extend: 'Proxmox.window.Edit',
+ alias: 'widget.pbsAddTotp',
+ mixins: ['Proxmox.Mixin.CBind'],
+
+ onlineHelp: 'user_mgmt',
+
+ modal: true,
+ resizable: false,
+ title: gettext('Add a TOTP login factor'),
+ width: 512,
+ layout: {
+ type: 'vbox',
+ align: 'stretch',
+ },
+
+ isAdd: true,
+ userid: undefined,
+ tfa_id: undefined,
+ fixedUser: false,
+
+ updateQrCode: function() {
+ let me = this;
+ let values = me.lookup('totp_form').getValues();
+ let algorithm = values.algorithm;
+ if (!algorithm) {
+ algorithm = 'SHA1';
+ }
+
+ let otpuri =
+ 'otpauth://totp/' + encodeURIComponent(values.userid) +
+ '?secret=' + values.secret +
+ '&period=' + values.step +
+ '&digits=' + values.digits +
+ '&algorithm=' + algorithm +
+ '&issuer=' + encodeURIComponent(values.issuer);
+
+ me.getController().getViewModel().set('otpuri', otpuri);
+ me.qrcode.makeCode(otpuri);
+ me.lookup('challenge').setVisible(true);
+ me.down('#qrbox').setVisible(true);
+ },
+
+ viewModel: {
+ data: {
+ valid: false,
+ secret: '',
+ otpuri: '',
+ },
+
+ formulas: {
+ secretEmpty: function(get) {
+ return get('secret').length === 0;
+ },
+ },
+ },
+
+ controller: {
+ xclass: 'Ext.app.ViewController',
+ control: {
+ 'field[qrupdate=true]': {
+ change: function() {
+ this.getView().updateQrCode();
+ },
+ },
+ 'field': {
+ validitychange: function(field, valid) {
+ let me = this;
+ let viewModel = me.getViewModel();
+ let form = me.lookup('totp_form');
+ let challenge = me.lookup('challenge');
+ let password = me.lookup('password');
+ viewModel.set('valid', form.isValid() && challenge.isValid() && password.isValid());
+ },
+ },
+ '#': {
+ show: function() {
+ let me = this;
+ let view = me.getView();
+
+ view.qrdiv = document.createElement('div');
+ view.qrcode = new QRCode(view.qrdiv, {
+ width: 256,
+ height: 256,
+ correctLevel: QRCode.CorrectLevel.M,
+ });
+ view.down('#qrbox').getEl().appendChild(view.qrdiv);
+
+ view.getController().randomizeSecret();
+
+ if (Proxmox.UserName === 'root@pam') {
+ view.lookup('password').setVisible(false);
+ view.lookup('password').setDisabled(true);
+ }
+ },
+ },
+ },
+
+ randomizeSecret: function() {
+ let me = this;
+ let rnd = new Uint8Array(32);
+ window.crypto.getRandomValues(rnd);
+ let data = '';
+ rnd.forEach(function(b) {
+ // secret must be base32, so just use the first 5 bits
+ b = b & 0x1f;
+ if (b < 26) {
+ // A..Z
+ data += String.fromCharCode(b + 0x41);
+ } else {
+ // 2..7
+ data += String.fromCharCode(b-26 + 0x32);
+ }
+ });
+ me.getViewModel().set('secret', data);
+ },
+ },
+
+ items: [
+ {
+ xtype: 'form',
+ layout: 'anchor',
+ border: false,
+ reference: 'totp_form',
+ fieldDefaults: {
+ anchor: '100%',
+ padding: '0 5',
+ },
+ items: [
+ {
+ xtype: 'pmxDisplayEditField',
+ name: 'userid',
+ cbind: {
+ editable: (get) => get('isAdd') && !get('fixedUser'),
+ },
+ fieldLabel: gettext('User'),
+ editConfig: {
+ xtype: 'pbsUserSelector',
+ allowBlank: false,
+ },
+ renderer: Ext.String.htmlEncode,
+ value: Proxmox.UserName,
+ qrupdate: true,
+ },
+ {
+ xtype: 'textfield',
+ fieldLabel: gettext('Description'),
+ allowBlank: false,
+ name: 'description',
+ maxLength: 256,
+ },
+ {
+ layout: 'hbox',
+ border: false,
+ padding: '0 0 5 0',
+ items: [{
+ xtype: 'textfield',
+ fieldLabel: gettext('Secret'),
+ emptyText: gettext('Unchanged'),
+ name: 'secret',
+ reference: 'tfa_secret',
+ regex: /^[A-Z2-7=]+$/,
+ regexText: 'Must be base32 [A-Z2-7=]',
+ maskRe: /[A-Z2-7=]/,
+ qrupdate: true,
+ bind: {
+ value: "{secret}",
+ },
+ flex: 4,
+ },
+ {
+ xtype: 'button',
+ text: gettext('Randomize'),
+ reference: 'randomize_button',
+ handler: 'randomizeSecret',
+ flex: 1,
+ }],
+ },
+ {
+ xtype: 'numberfield',
+ fieldLabel: gettext('Time period'),
+ name: 'step',
+ // Google Authenticator ignores this and generates bogus data
+ hidden: true,
+ value: 30,
+ minValue: 10,
+ qrupdate: true,
+ },
+ {
+ xtype: 'numberfield',
+ fieldLabel: gettext('Digits'),
+ name: 'digits',
+ value: 6,
+ // Google Authenticator ignores this and generates bogus data
+ hidden: true,
+ minValue: 6,
+ maxValue: 8,
+ qrupdate: true,
+ },
+ {
+ xtype: 'textfield',
+ fieldLabel: gettext('Issuer Name'),
+ name: 'issuer',
+ value: `Proxmox Backup Server - ${Proxmox.NodeName}`,
+ qrupdate: true,
+ },
+ ],
+ },
+ {
+ xtype: 'box',
+ itemId: 'qrbox',
+ visible: false, // will be enabled when generating a qr code
+ bind: {
+ visible: '{!secretEmpty}',
+ },
+ style: {
+ 'background-color': 'white',
+ 'margin-left': 'auto',
+ 'margin-right': 'auto',
+ padding: '5px',
+ width: '266px',
+ height: '266px',
+ },
+ },
+ {
+ xtype: 'textfield',
+ fieldLabel: gettext('Verification Code'),
+ allowBlank: false,
+ reference: 'challenge',
+ name: 'challenge',
+ bind: {
+ disabled: '{!showTOTPVerifiction}',
+ visible: '{showTOTPVerifiction}',
+ },
+ padding: '0 5',
+ emptyText: gettext('Scan QR code and enter TOTP auth. code to verify'),
+ },
+ {
+ xtype: 'textfield',
+ inputType: 'password',
+ fieldLabel: gettext('Password'),
+ minLength: 5,
+ reference: 'password',
+ name: 'password',
+ allowBlank: false,
+ validateBlank: true,
+ padding: '0 0 5 5',
+ emptyText: gettext('verify current password'),
+ },
+ ],
+
+ initComponent: function() {
+ let me = this;
+ me.url = '/api2/extjs/access/tfa/';
+ me.method = 'POST';
+ me.callParent();
+ },
+
+ getValues: function(dirtyOnly) {
+ let me = this;
+ let viewmodel = me.getController().getViewModel();
+
+ let values = me.callParent(arguments);
+
+ let uid = encodeURIComponent(values.userid);
+ me.url = `/api2/extjs/access/tfa/${uid}`;
+ delete values.userid;
+
+ let data = {
+ description: values.description,
+ type: "totp",
+ totp: viewmodel.get('otpuri'),
+ value: values.challenge,
+ };
+
+ if (values.password) {
+ data.password = values.password;
+ }
+
+ return data;
+ },
+});
diff --git a/www/window/AddWebauthn.js b/www/window/AddWebauthn.js
new file mode 100644
index 00000000..2c64dd0c
--- /dev/null
+++ b/www/window/AddWebauthn.js
@@ -0,0 +1,193 @@
+Ext.define('PBS.window.AddWebauthn', {
+ extend: 'Ext.window.Window',
+ alias: 'widget.pbsAddWebauthn',
+ mixins: ['Proxmox.Mixin.CBind'],
+
+ onlineHelp: 'user_mgmt',
+
+ modal: true,
+ resizable: false,
+ title: gettext('Add a Webauthn login token'),
+ width: 512,
+
+ user: undefined,
+ fixedUser: false,
+
+ initComponent: function() {
+ let me = this;
+ me.callParent();
+ Ext.GlobalEvents.fireEvent('proxmoxShowHelp', me.onlineHelp);
+ },
+
+ viewModel: {
+ data: {
+ valid: false,
+ },
+ },
+
+ controller: {
+ xclass: 'Ext.app.ViewController',
+
+ control: {
+ 'field': {
+ validitychange: function(field, valid) {
+ let me = this;
+ let viewmodel = me.getViewModel();
+ let form = me.lookup('webauthn_form');
+ viewmodel.set('valid', form.isValid());
+ },
+ },
+ '#': {
+ show: function() {
+ let me = this;
+ let view = me.getView();
+
+ if (Proxmox.UserName === 'root@pam') {
+ view.lookup('password').setVisible(false);
+ view.lookup('password').setDisabled(true);
+ }
+ },
+ },
+ },
+
+ registerWebauthn: async function() {
+ let me = this;
+ let values = me.lookup('webauthn_form').getValues();
+ values.type = "webauthn";
+
+ let userid = values.user;
+ delete values.user;
+
+ try {
+ let register_response = await PBS.Async.api2({
+ url: `/api2/extjs/access/tfa/${userid}`,
+ method: 'POST',
+ params: values,
+ });
+
+ let data = register_response.result.data;
+ if (!data.challenge) {
+ throw "server did not respond with a challenge";
+ }
+
+ let challenge_obj = JSON.parse(data.challenge);
+
+ // Fix this up before passing it to the browser, but keep a copy of the original
+ // string to pass in the response:
+ let challenge_str = challenge_obj.publicKey.challenge;
+ challenge_obj.publicKey.challenge = PBS.Utils.base64url_to_bytes(challenge_str);
+ challenge_obj.publicKey.user.id =
+ PBS.Utils.base64url_to_bytes(challenge_obj.publicKey.user.id);
+
+ let msg = Ext.Msg.show({
+ title: `Webauthn: ${gettext('Setup')}`,
+ message: gettext('Please press the button on your Webauthn Device'),
+ buttons: [],
+ });
+
+ let token_response = await navigator.credentials.create(challenge_obj);
+
+ // We cannot pass ArrayBuffers to the API, so extract & convert the data.
+ let response = {
+ id: token_response.id,
+ type: token_response.type,
+ rawId: PBS.Utils.bytes_to_base64url(token_response.rawId),
+ response: {
+ attestationObject: PBS.Utils.bytes_to_base64url(
+ token_response.response.attestationObject,
+ ),
+ clientDataJSON: PBS.Utils.bytes_to_base64url(
+ token_response.response.clientDataJSON,
+ ),
+ },
+ };
+
+ msg.close();
+
+ let params = {
+ type: "webauthn",
+ challenge: challenge_str,
+ value: JSON.stringify(response),
+ };
+
+ if (values.password) {
+ params.password = values.password;
+ }
+
+ await PBS.Async.api2({
+ url: `/api2/extjs/access/tfa/${userid}`,
+ method: 'POST',
+ params,
+ });
+ } catch (error) {
+ console.error(error); // for debugging if it's not displayable...
+ Ext.Msg.alert(gettext('Error'), error);
+ }
+
+ me.getView().close();
+ },
+ },
+
+ items: [
+ {
+ xtype: 'form',
+ reference: 'webauthn_form',
+ layout: 'anchor',
+ bodyPadding: 10,
+ fieldDefaults: {
+ anchor: '100%',
+ },
+ items: [
+ {
+ xtype: 'pmxDisplayEditField',
+ name: 'user',
+ cbind: {
+ editable: (get) => !get('fixedUser'),
+ },
+ fieldLabel: gettext('User'),
+ editConfig: {
+ xtype: 'pbsUserSelector',
+ allowBlank: false,
+ },
+ renderer: Ext.String.htmlEncode,
+ value: Proxmox.UserName,
+ },
+ {
+ xtype: 'textfield',
+ fieldLabel: gettext('Description'),
+ allowBlank: false,
+ name: 'description',
+ maxLength: 256,
+ emptyText: gettext('a short distinguishing description'),
+ },
+ {
+ xtype: 'textfield',
+ inputType: 'password',
+ fieldLabel: gettext('Password'),
+ minLength: 5,
+ reference: 'password',
+ name: 'password',
+ allowBlank: false,
+ validateBlank: true,
+ padding: '0 0 5 5',
+ emptyText: gettext('verify current password'),
+ },
+ ],
+ },
+ ],
+
+ buttons: [
+ {
+ xtype: 'proxmoxHelpButton',
+ },
+ '->',
+ {
+ xtype: 'button',
+ text: gettext('Register Webauthn Device'),
+ handler: 'registerWebauthn',
+ bind: {
+ disabled: '{!valid}',
+ },
+ },
+ ],
+});
diff --git a/www/window/TfaEdit.js b/www/window/TfaEdit.js
new file mode 100644
index 00000000..182da33b
--- /dev/null
+++ b/www/window/TfaEdit.js
@@ -0,0 +1,92 @@
+Ext.define('PBS.window.TfaEdit', {
+ extend: 'Proxmox.window.Edit',
+ alias: 'widget.pbsTfaEdit',
+ mixins: ['Proxmox.Mixin.CBind'],
+
+ onlineHelp: 'user_mgmt',
+
+ modal: true,
+ resizable: false,
+ title: gettext("Modify a TFA entry's description"),
+ width: 512,
+
+ layout: {
+ type: 'vbox',
+ align: 'stretch',
+ },
+
+ cbindData: function(initialConfig) {
+ let me = this;
+
+ let tfa_id = initialConfig['tfa-id'];
+ me.tfa_id = tfa_id;
+ me.defaultFocus = 'textfield[name=description]';
+ me.url = `/api2/extjs/access/tfa/${tfa_id}`;
+ me.method = 'PUT';
+ me.autoLoad = true;
+ return {};
+ },
+
+ initComponent: function() {
+ let me = this;
+ me.callParent();
+
+ if (Proxmox.UserName === 'root@pam') {
+ me.lookup('password').setVisible(false);
+ me.lookup('password').setDisabled(true);
+ }
+
+ let userid = me.tfa_id.split('/')[0];
+ me.lookup('userid').setValue(userid);
+ },
+
+ items: [
+ {
+ xtype: 'displayfield',
+ reference: 'userid',
+ editable: false,
+ fieldLabel: gettext('User'),
+ editConfig: {
+ xtype: 'pbsUserSelector',
+ allowBlank: false,
+ },
+ value: Proxmox.UserName,
+ },
+ {
+ xtype: 'proxmoxtextfield',
+ name: 'description',
+ allowBlank: false,
+ fieldLabel: gettext('Description'),
+ },
+ {
+ xtype: 'proxmoxcheckbox',
+ fieldLabel: gettext('Enabled'),
+ name: 'enable',
+ uncheckedValue: 0,
+ defaultValue: 1,
+ checked: true,
+ },
+ {
+ xtype: 'textfield',
+ inputType: 'password',
+ fieldLabel: gettext('Password'),
+ minLength: 5,
+ reference: 'password',
+ name: 'password',
+ allowBlank: false,
+ validateBlank: true,
+ padding: '0 0 5 5',
+ emptyText: gettext('verify current password'),
+ },
+ ],
+
+ getValues: function() {
+ var me = this;
+
+ var values = me.callParent(arguments);
+
+ delete values.userid;
+
+ return values;
+ },
+});
--
2.20.1
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [pbs-devel] [RFC backup 6/6] gui: tfa support
2020-11-19 14:56 ` [pbs-devel] [RFC backup 6/6] gui: tfa support Wolfgang Bumiller
@ 2020-11-24 9:42 ` Wolfgang Bumiller
2020-11-24 9:51 ` Thomas Lamprecht
0 siblings, 1 reply; 23+ messages in thread
From: Wolfgang Bumiller @ 2020-11-24 9:42 UTC (permalink / raw)
To: pbs-devel
2 notes:
On Thu, Nov 19, 2020 at 03:56:08PM +0100, Wolfgang Bumiller wrote:
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
> www/LoginView.js | 323 +++++++++++++++++++++++++++++++++--
> www/Makefile | 6 +
> www/OnlineHelpInfo.js | 36 ----
> www/Utils.js | 59 +++++++
> www/config/TfaView.js | 322 ++++++++++++++++++++++++++++++++++
> www/index.hbs | 1 +
> www/panel/AccessControl.js | 6 +
> www/window/AddTfaRecovery.js | 211 +++++++++++++++++++++++
> www/window/AddTotp.js | 283 ++++++++++++++++++++++++++++++
> www/window/AddWebauthn.js | 193 +++++++++++++++++++++
> www/window/TfaEdit.js | 92 ++++++++++
> 11 files changed, 1478 insertions(+), 54 deletions(-)
> create mode 100644 www/config/TfaView.js
> create mode 100644 www/window/AddTfaRecovery.js
> create mode 100644 www/window/AddTotp.js
> create mode 100644 www/window/AddWebauthn.js
> create mode 100644 www/window/TfaEdit.js
>
> diff --git a/www/Makefile b/www/Makefile
> index 1df2195a..86e0516e 100644
> --- a/www/Makefile
> +++ b/www/Makefile
> @@ -16,12 +16,16 @@ JSSRC= \
> data/RunningTasksStore.js \
> button/TaskButton.js \
> config/UserView.js \
> + config/TfaView.js \
> config/TokenView.js \
> config/RemoteView.js \
> config/ACLView.js \
> config/SyncView.js \
> config/VerifyView.js \
> window/ACLEdit.js \
> + window/AddTfaRecovery.js \
> + window/AddTotp.js \
> + window/AddWebauthn.js \
> window/BackupFileDownloader.js \
> window/BackupGroupChangeOwner.js \
> window/CreateDirectory.js \
> @@ -34,6 +38,7 @@ JSSRC= \
> window/UserEdit.js \
> window/UserPassword.js \
> window/TokenEdit.js \
> + window/TfaEdit.js \
> window/VerifyJobEdit.js \
> window/ZFSCreate.js \
> dashboard/DataStoreStatistics.js \
> @@ -100,6 +105,7 @@ install: js/proxmox-backup-gui.js css/ext6-pbs.css index.hbs
> install -m644 index.hbs $(DESTDIR)$(JSDIR)/
> install -dm755 $(DESTDIR)$(JSDIR)/js
> install -m644 js/proxmox-backup-gui.js $(DESTDIR)$(JSDIR)/js/
> + install -m 0644 qrcode.min.js $(DESTDIR)$(JSDIR)/
FYI this is a leftover from before the this was packaged separately,
needs to be removed when building a package without the file present.
> install -dm755 $(DESTDIR)$(JSDIR)/css
> install -m644 css/ext6-pbs.css $(DESTDIR)$(JSDIR)/css/
> install -dm755 $(DESTDIR)$(JSDIR)/images
> diff --git a/www/OnlineHelpInfo.js b/www/OnlineHelpInfo.js
> index aee73bf6..c54912d8 100644
> --- a/www/OnlineHelpInfo.js
> +++ b/www/OnlineHelpInfo.js
The changs in this file were a build artifact...?
> @@ -75,42 +75,6 @@ const proxmoxOnlineHelpInfo = {
> "link": "/docs/pve-integration.html#pve-integration",
> "title": "`Proxmox VE`_ Integration"
> },
> - "rst-primer": {
> - "link": "/docs/reStructuredText-primer.html#rst-primer",
> - "title": "reStructuredText Primer"
> - },
> - "rst-inline-markup": {
> - "link": "/docs/reStructuredText-primer.html#rst-inline-markup",
> - "title": "Inline markup"
> - },
> - "rst-literal-blocks": {
> - "link": "/docs/reStructuredText-primer.html#rst-literal-blocks",
> - "title": "Literal blocks"
> - },
> - "rst-doctest-blocks": {
> - "link": "/docs/reStructuredText-primer.html#rst-doctest-blocks",
> - "title": "Doctest blocks"
> - },
> - "rst-tables": {
> - "link": "/docs/reStructuredText-primer.html#rst-tables",
> - "title": "Tables"
> - },
> - "rst-field-lists": {
> - "link": "/docs/reStructuredText-primer.html#rst-field-lists",
> - "title": "Field Lists"
> - },
> - "rst-roles-alt": {
> - "link": "/docs/reStructuredText-primer.html#rst-roles-alt",
> - "title": "Roles"
> - },
> - "rst-directives": {
> - "link": "/docs/reStructuredText-primer.html#rst-directives",
> - "title": "Directives"
> - },
> - "html-meta": {
> - "link": "/docs/reStructuredText-primer.html#html-meta",
> - "title": "HTML Metadata"
> - },
> "storage-disk-management": {
> "link": "/docs/storage.html#storage-disk-management",
> "title": "Disk Management"
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [pbs-devel] [RFC backup 0/6] Two factor authentication
2020-11-19 14:56 [pbs-devel] [RFC backup 0/6] Two factor authentication Wolfgang Bumiller
` (5 preceding siblings ...)
2020-11-19 14:56 ` [pbs-devel] [RFC backup 6/6] gui: tfa support Wolfgang Bumiller
@ 2020-12-02 10:56 ` Oguz Bektas
2020-12-02 12:27 ` Thomas Lamprecht
6 siblings, 1 reply; 23+ messages in thread
From: Oguz Bektas @ 2020-12-02 10:56 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
hi,
we talked with wolfgang off-list about some issues, so here are
some recommendations for the next version:
1. increase the length of recovery codes for bruteforce mitigation
most websites use 12-16 characters for the length of recovery keys.
2. do not store recovery codes in cleartext (hash them instead, we thought
hmac-sha256 is fine). the reason being that recovery codes can bypass
other tfa methods so they shouldn't be visible
3. don't store all the tfa information in a single json file.
current version uses a single /etc/proxmox-backup/tfa.json file
which holds all the tfa info for all the users. this is a single point
of failure because:
- file can be corrupted, causing tfa to break for everyone (no more logins)
- file could get deleted, disabling/bypassing 2fa for everyone
- file could get leaked in a backup etc., giving everyone's tfa secrets
and/or recovery keys to attackers (bypass everything)
better is to at least create a file for each user:
/etc/proxmox-backup/tfa/<username>.json or similar
this way the damage is contained if for example the config breaks
because of incorrect deserialization etc.
4. html/js injection in the description field on gui (fixed on staff
repo already)
5. notify user if more than X failed tfa attempts (password is already
compromised at this point, so it's important to notify) and block IP
for certain amount of time (fail2ban?)
5.b also if recovery keys are available, limit amount of TOTP attempts
for that user
review still going on, but i figured it's good to have a feedback loop
:)
On Thu, Nov 19, 2020 at 03:56:02PM +0100, Wolfgang Bumiller wrote:
> This series is a first RFC for two factor authentication.
>
> Notes:
> * Backend contains support for TOTP, Webauthn, Recovery keys and U2F.
> * The webauthn-rs crate introduces new dependencies we need to package:
> - `half`
> - `serde-cbor`
> For our internal rust packaging I pushed the current debcargo-conf
> changes to my `staff/w.bumiller/debcargo-conf` repo, branch `webauthn-rs`.
>
> some extra (already-packaged) deps will be pulled in along with them:
> $ cargo update
> Adding getrandom v0.1.14
> Adding half v1.6.0
> Adding nom v4.2.3
> Adding ppv-lite86 v0.2.6
> Adding rand v0.7.2
> Adding rand_chacha v0.2.2
> Adding rand_core v0.5.1
> Adding rand_hc v0.2.0
> Adding serde_bytes v0.11.5
> Adding serde_cbor v0.11.1
> Adding thiserror v1.0.15
> Adding thiserror-impl v1.0.15
> Adding webauthn-rs v0.2.5
>
> * I wrote u2f before webauthn and left it in there unused because:
> * we may want to move the code out to be integrated to PVE and PBS as
> well for webauthn
> * if we do: the webauthn-rs crate doesn't seem to provide a way
> forward to using existin u2f credentials, so to not break those
> we'll need the u2f backend.
>
> * The GUI does not use U2F (see above). (I do have code for it if for
> some reason we do want that).
>
> * The GUI code is probably super weird. Some windows might look clunky,
> but for me they always do currently since I'm working with
> non-standard dpi monitor settings... so I can't really tell :-P
>
> * I introduced some `async` code into the GUI because the webauthn api
> uses Promises and extjs doesn't seem to have issues with that...
>
> * The TFA configuration is currently a single json file.
>
> * While writing this mail I realized I still want to change the way
> webauthn credentials are being serialized, but that's not important
> for a first draft to look through ;-)
>
> Wolfgang Bumiller (6):
> add tools::serde_filter submodule
> config: add tfa configuration
> api: tfa management and login
> depend on libjs-qrcodejs
> proxy: expose qrcodejs
> gui: tfa support
>
> Cargo.toml | 1 +
> debian/control.in | 1 +
> src/api2/access.rs | 171 ++++--
> src/api2/access/tfa.rs | 567 +++++++++++++++++
> src/bin/proxmox-backup-proxy.rs | 1 +
> src/config.rs | 1 +
> src/config/tfa.rs | 1017 +++++++++++++++++++++++++++++++
> src/server.rs | 2 +
> src/server/rest.rs | 5 +-
> src/server/ticket.rs | 77 +++
> src/tools.rs | 1 +
> src/tools/serde_filter.rs | 97 +++
> www/LoginView.js | 323 +++++++++-
> www/Makefile | 6 +
> www/OnlineHelpInfo.js | 36 --
> www/Utils.js | 59 ++
> www/config/TfaView.js | 322 ++++++++++
> www/index.hbs | 1 +
> www/panel/AccessControl.js | 6 +
> www/window/AddTfaRecovery.js | 211 +++++++
> www/window/AddTotp.js | 283 +++++++++
> www/window/AddWebauthn.js | 193 ++++++
> www/window/TfaEdit.js | 92 +++
> 23 files changed, 3357 insertions(+), 116 deletions(-)
> create mode 100644 src/api2/access/tfa.rs
> create mode 100644 src/config/tfa.rs
> create mode 100644 src/server/ticket.rs
> create mode 100644 src/tools/serde_filter.rs
> create mode 100644 www/config/TfaView.js
> create mode 100644 www/window/AddTfaRecovery.js
> create mode 100644 www/window/AddTotp.js
> create mode 100644 www/window/AddWebauthn.js
> create mode 100644 www/window/TfaEdit.js
>
> --
> 2.20.1
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [pbs-devel] [RFC backup 0/6] Two factor authentication
2020-12-02 10:56 ` [pbs-devel] [RFC backup 0/6] Two factor authentication Oguz Bektas
@ 2020-12-02 12:27 ` Thomas Lamprecht
2020-12-02 12:34 ` Thomas Lamprecht
2020-12-02 12:35 ` Oguz Bektas
0 siblings, 2 replies; 23+ messages in thread
From: Thomas Lamprecht @ 2020-12-02 12:27 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Oguz Bektas
Hi,
thanks for taking a look, some comments regarding your feedback.
On 02.12.20 11:56, Oguz Bektas wrote:
> we talked with wolfgang off-list about some issues, so here are
> some recommendations for the next version:
>
> 1. increase the length of recovery codes for bruteforce mitigation
>
> most websites use 12-16 characters for the length of recovery keys.
makes sense
>
> 2. do not store recovery codes in cleartext (hash them instead, we thought
> hmac-sha256 is fine). the reason being that recovery codes can bypass
> other tfa methods so they shouldn't be visible
make sense, would expect them to be hashed
>
> 3. don't store all the tfa information in a single json file.
>
makes no sense to me, any reason you mention below can happen to arbitrary
files, so just adds complexity while not gaining anything.
> current version uses a single /etc/proxmox-backup/tfa.json file
> which holds all the tfa info for all the users. this is a single point
> of failure because:
> - file can be corrupted, causing tfa to break for everyone (no more logins)
> - file could get deleted, disabling/bypassing 2fa for everyone
> - file could get leaked in a backup etc., giving everyone's tfa secrets
> and/or recovery keys to attackers (bypass everything)
>
> better is to at least create a file for each user:
> /etc/proxmox-backup/tfa/<username>.json or similar
>
> this way the damage is contained if for example the config breaks
> because of incorrect deserialization etc.
Why would deserialisation be incorrect for one single file but magically
works if multiple files? Makes no sense.
>
> 4. html/js injection in the description field on gui (fixed on staff
> repo already)
>
Yeah, as always, Ext.String.htmlEncode is your friend ;)
> 5. notify user if more than X failed tfa attempts (password is already
> compromised at this point, so it's important to notify) and block IP
> for certain amount of time (fail2ban?)
we do not setup fail2ban but any admin can already if wished. Notification
can only work if the user has setup a mail in the first place - but yes, sou
>
> 5.b also if recovery keys are available, limit amount of TOTP attempts
> for that user
what?
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [pbs-devel] [RFC backup 0/6] Two factor authentication
2020-12-02 12:27 ` Thomas Lamprecht
@ 2020-12-02 12:34 ` Thomas Lamprecht
2020-12-02 12:48 ` Oguz Bektas
2020-12-02 12:35 ` Oguz Bektas
1 sibling, 1 reply; 23+ messages in thread
From: Thomas Lamprecht @ 2020-12-02 12:34 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Oguz Bektas
On 02.12.20 13:27, Thomas Lamprecht wrote:
> - file could get leaked in a backup etc., giving everyone's tfa secrets
> and/or recovery keys to attackers (bypass everything)
for the record, that does *not* "bypass everything", it's a *second* factor
after all. Further, if recovery keys are hashed they do not leak information.
For others it varies, but I do not like that sort of blanket statement without
implying any reasonable vector at all, we and most unix system have such
information in one place /etc/shadow, our shadow in /etc/pve/ and consorts,
it needs clear documentation about what files are sensible (you should send a
patch for that) but that's it.
(and as said, splitting it up will not avoid leaking all of them in a backup vs. just
one of it).
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [pbs-devel] [RFC backup 0/6] Two factor authentication
2020-12-02 12:34 ` Thomas Lamprecht
@ 2020-12-02 12:48 ` Oguz Bektas
2020-12-02 12:59 ` Wolfgang Bumiller
0 siblings, 1 reply; 23+ messages in thread
From: Oguz Bektas @ 2020-12-02 12:48 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On Wed, Dec 02, 2020 at 01:34:25PM +0100, Thomas Lamprecht wrote:
> On 02.12.20 13:27, Thomas Lamprecht wrote:
> > - file could get leaked in a backup etc., giving everyone's tfa secrets
> > and/or recovery keys to attackers (bypass everything)
>
> for the record, that does *not* "bypass everything", it's a *second* factor
> after all.
yes "bypass everything" was a bit of overstatement on my end.. :)
> Further, if recovery keys are hashed they do not leak information.
the totp secrets are stored without hashing or encryption so it'd bypass
that one if file is leaked etc.
> For others it varies, but I do not like that sort of blanket statement without
> implying any reasonable vector at all, we and most unix system have such
> information in one place /etc/shadow, our shadow in /etc/pve/ and consorts,
> it needs clear documentation about what files are sensible (you should send a
> patch for that) but that's it.
> (and as said, splitting it up will not avoid leaking all of them in a backup vs. just
> one of it).
i was also thinking if it's a good idea to use a symmetric algorithm to
encrypt the json file with that user's password. it would help in
backup leak or similar cases, but could also be overhead (need to
decrypt/encrypt the file everytime it's changed, need to re-encrypt if
user changes password etc.)
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [pbs-devel] [RFC backup 0/6] Two factor authentication
2020-12-02 12:48 ` Oguz Bektas
@ 2020-12-02 12:59 ` Wolfgang Bumiller
2020-12-02 13:08 ` Oguz Bektas
0 siblings, 1 reply; 23+ messages in thread
From: Wolfgang Bumiller @ 2020-12-02 12:59 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
> On 12/02/2020 1:48 PM Oguz Bektas <o.bektas@proxmox.com> wrote:
>
>
> On Wed, Dec 02, 2020 at 01:34:25PM +0100, Thomas Lamprecht wrote:
> > On 02.12.20 13:27, Thomas Lamprecht wrote:
> > > - file could get leaked in a backup etc., giving everyone's tfa secrets
> > > and/or recovery keys to attackers (bypass everything)
> >
> > for the record, that does *not* "bypass everything", it's a *second* factor
> > after all.
>
> yes "bypass everything" was a bit of overstatement on my end.. :)
> > Further, if recovery keys are hashed they do not leak information.
> the totp secrets are stored without hashing or encryption so it'd bypass
> that one if file is leaked etc.
> > For others it varies, but I do not like that sort of blanket statement without
> > implying any reasonable vector at all, we and most unix system have such
> > information in one place /etc/shadow, our shadow in /etc/pve/ and consorts,
> > it needs clear documentation about what files are sensible (you should send a
> > patch for that) but that's it.
> > (and as said, splitting it up will not avoid leaking all of them in a backup vs. just
> > one of it).
> i was also thinking if it's a good idea to use a symmetric algorithm to
> encrypt the json file with that user's password. it would help in
> backup leak or similar cases, but could also be overhead (need to
> decrypt/encrypt the file everytime it's changed, need to re-encrypt if
> user changes password etc.)
If you mean the hash we store, then I'm against it, simply because the key
is lying right next to it.
If you mean the user's *actual* password, then
* we'd need to query the user's password even to just list the current TFA entries
* a lost password automatically means you have to re-register your second factors.
(Not *much* of a problem, but kind of weird.)
* root could not read nor modify a user's tfa entries (also not a problem, but weird)
A mixed approach would keep the description & metadata in plaintext and store the
secrets in an encrypted form with an AAD cipher. But the only thing benefiting from
this really would be the TOTP entries. WA uses asymmetric cryptography already,
and if we hash the recovery keys, those should be okay, too.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [pbs-devel] [RFC backup 0/6] Two factor authentication
2020-12-02 12:59 ` Wolfgang Bumiller
@ 2020-12-02 13:08 ` Oguz Bektas
0 siblings, 0 replies; 23+ messages in thread
From: Oguz Bektas @ 2020-12-02 13:08 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On Wed, Dec 02, 2020 at 01:59:31PM +0100, Wolfgang Bumiller wrote:
>
> > On 12/02/2020 1:48 PM Oguz Bektas <o.bektas@proxmox.com> wrote:
> >
> >
> > On Wed, Dec 02, 2020 at 01:34:25PM +0100, Thomas Lamprecht wrote:
> > > On 02.12.20 13:27, Thomas Lamprecht wrote:
> > > > - file could get leaked in a backup etc., giving everyone's tfa secrets
> > > > and/or recovery keys to attackers (bypass everything)
> > >
> > > for the record, that does *not* "bypass everything", it's a *second* factor
> > > after all.
> >
> > yes "bypass everything" was a bit of overstatement on my end.. :)
> > > Further, if recovery keys are hashed they do not leak information.
> > the totp secrets are stored without hashing or encryption so it'd bypass
> > that one if file is leaked etc.
> > > For others it varies, but I do not like that sort of blanket statement without
> > > implying any reasonable vector at all, we and most unix system have such
> > > information in one place /etc/shadow, our shadow in /etc/pve/ and consorts,
> > > it needs clear documentation about what files are sensible (you should send a
> > > patch for that) but that's it.
> > > (and as said, splitting it up will not avoid leaking all of them in a backup vs. just
> > > one of it).
> > i was also thinking if it's a good idea to use a symmetric algorithm to
> > encrypt the json file with that user's password. it would help in
> > backup leak or similar cases, but could also be overhead (need to
> > decrypt/encrypt the file everytime it's changed, need to re-encrypt if
> > user changes password etc.)
>
> If you mean the hash we store, then I'm against it, simply because the key
> is lying right next to it.
> If you mean the user's *actual* password, then
yes
> * we'd need to query the user's password even to just list the current TFA entries
> * a lost password automatically means you have to re-register your second factors.
> (Not *much* of a problem, but kind of weird.)
> * root could not read nor modify a user's tfa entries (also not a problem, but weird)
yeah...
>
> A mixed approach would keep the description & metadata in plaintext and store the
> secrets in an encrypted form with an AAD cipher. But the only thing benefiting from
> this really would be the TOTP entries. WA uses asymmetric cryptography already,
> and if we hash the recovery keys, those should be okay, too.
but this actually would make sense, i like that approach as it would
solve the issues you previously pointed out and keep it relatively
secure (very much in comparison to just keeping the secrets in
plaintext)
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [pbs-devel] [RFC backup 0/6] Two factor authentication
2020-12-02 12:27 ` Thomas Lamprecht
2020-12-02 12:34 ` Thomas Lamprecht
@ 2020-12-02 12:35 ` Oguz Bektas
2020-12-02 12:51 ` Wolfgang Bumiller
2020-12-02 13:07 ` Thomas Lamprecht
1 sibling, 2 replies; 23+ messages in thread
From: Oguz Bektas @ 2020-12-02 12:35 UTC (permalink / raw)
To: Thomas Lamprecht; +Cc: Proxmox Backup Server development discussion
On Wed, Dec 02, 2020 at 01:27:47PM +0100, Thomas Lamprecht wrote:
> Hi,
>
> thanks for taking a look, some comments regarding your feedback.
>
> On 02.12.20 11:56, Oguz Bektas wrote:
> > we talked with wolfgang off-list about some issues, so here are
> > some recommendations for the next version:
> >
> > 1. increase the length of recovery codes for bruteforce mitigation
> >
> > most websites use 12-16 characters for the length of recovery keys.
>
> makes sense
>
> >
> > 2. do not store recovery codes in cleartext (hash them instead, we thought
> > hmac-sha256 is fine). the reason being that recovery codes can bypass
> > other tfa methods so they shouldn't be visible
>
> make sense, would expect them to be hashed
>
> >
> > 3. don't store all the tfa information in a single json file.
> >
>
> makes no sense to me, any reason you mention below can happen to arbitrary
> files, so just adds complexity while not gaining anything.
>
> > current version uses a single /etc/proxmox-backup/tfa.json file
> > which holds all the tfa info for all the users. this is a single point
> > of failure because:
> > - file can be corrupted, causing tfa to break for everyone (no more logins)
> > - file could get deleted, disabling/bypassing 2fa for everyone
> > - file could get leaked in a backup etc., giving everyone's tfa secrets
> > and/or recovery keys to attackers (bypass everything)
> >
> > better is to at least create a file for each user:
> > /etc/proxmox-backup/tfa/<username>.json or similar
> >
> > this way the damage is contained if for example the config breaks
> > because of incorrect deserialization etc.
>
> Why would deserialisation be incorrect for one single file but magically
> works if multiple files? Makes no sense.
of course this can happen on arbitrary files... i don't see why it
would add any complexity to use multiple files though (actually makes it
simpler imo). the reasoning behind this was to avoid a single point of
failure like i explained:
multiple files for users -> only that user is affected by broken config,
other users can log in
single file for all users -> all users affected if config breaks and
nobody can log in
so the point wasn't to magically fix (potential) incorrect deserialization but to
reduce breakage in case something like that happens.
>
> >
> > 4. html/js injection in the description field on gui (fixed on staff
> > repo already)
> >
>
> Yeah, as always, Ext.String.htmlEncode is your friend ;)
:)
>
> > 5. notify user if more than X failed tfa attempts (password is already
> > compromised at this point, so it's important to notify) and block IP
> > for certain amount of time (fail2ban?)
>
> we do not setup fail2ban but any admin can already if wished. Notification
> can only work if the user has setup a mail in the first place - but yes, sou
yes, but imo 2fa is more sensitive to bruteforcing than regular
passwords so it would make sense to limit it by default
>
> >
> > 5.b also if recovery keys are available, limit amount of TOTP attempts
> > for that user
>
> what?
>
if a user sets up TOTP + recovery keys, then it would make sense to lock
account in case of a lot of auth attempts with TOTP, until recovery key
is entered (afaik this is a common mechanism). but maybe just
notifying the user is enough as well.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [pbs-devel] [RFC backup 0/6] Two factor authentication
2020-12-02 12:35 ` Oguz Bektas
@ 2020-12-02 12:51 ` Wolfgang Bumiller
2020-12-02 13:15 ` Thomas Lamprecht
2020-12-02 13:07 ` Thomas Lamprecht
1 sibling, 1 reply; 23+ messages in thread
From: Wolfgang Bumiller @ 2020-12-02 12:51 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
> On 12/02/2020 1:35 PM Oguz Bektas <o.bektas@proxmox.com> wrote:
>
>
> On Wed, Dec 02, 2020 at 01:27:47PM +0100, Thomas Lamprecht wrote:
> > > 2. do not store recovery codes in cleartext (hash them instead, we thought
> > > hmac-sha256 is fine). the reason being that recovery codes can bypass
> > > other tfa methods so they shouldn't be visible
> >
> > make sense, would expect them to be hashed
FWIW TOTP secrets can't be hashes since they're supposed to be,
well, a shared secret
> > >
> > > 3. don't store all the tfa information in a single json file.
> > >
> >
> > makes no sense to me, any reason you mention below can happen to arbitrary
> > files, so just adds complexity while not gaining anything.
Complexity is the wrong argument. The question is mainly whether we prefer
lots of small or one big file. For PBS it's not even that important. It'll
be more important when we add bindings for this to PVE where the file sizes
are limited.
With a file per user it's also easier for an admin to work on the files
directly. And if we want to add counters, limitations or eg. store date & ip
of the last time an entry was used, we won't be locking one big file at login
time. Not that I expect millions of concurrent logins to be happening ;-)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [pbs-devel] [RFC backup 0/6] Two factor authentication
2020-12-02 12:51 ` Wolfgang Bumiller
@ 2020-12-02 13:15 ` Thomas Lamprecht
0 siblings, 0 replies; 23+ messages in thread
From: Thomas Lamprecht @ 2020-12-02 13:15 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Wolfgang Bumiller
On 02.12.20 13:51, Wolfgang Bumiller wrote:
>> On 12/02/2020 1:35 PM Oguz Bektas <o.bektas@proxmox.com> wrote:
>>
>>
>> On Wed, Dec 02, 2020 at 01:27:47PM +0100, Thomas Lamprecht wrote:
>>>> 2. do not store recovery codes in cleartext (hash them instead, we thought
>>>> hmac-sha256 is fine). the reason being that recovery codes can bypass
>>>> other tfa methods so they shouldn't be visible
>>>
>>> make sense, would expect them to be hashed
>
> FWIW TOTP secrets can't be hashes since they're supposed to be,
> well, a shared secret
yeah sure, above talks about recovery keys though.
>
>>>>
>>>> 3. don't store all the tfa information in a single json file.
>>>>
>>>
>>> makes no sense to me, any reason you mention below can happen to arbitrary
>>> files, so just adds complexity while not gaining anything.
>
> Complexity is the wrong argument. The question is mainly whether we prefer
> lots of small or one big file. For PBS it's not even that important. It'll
> be more important when we add bindings for this to PVE where the file sizes
> are limited.
No complexity is seldom the wrong argument, it may not be enough on its own though.
>
> With a file per user it's also easier for an admin to work on the files
> directly. And if we want to add counters, limitations or eg. store date & ip
> of the last time an entry was used, we won't be locking one big file at login
> time. Not that I expect millions of concurrent logins to be happening ;-)
We create a confusing split view, all user info are concentrated into single files
per type, user.cfg, acl.cfg, shadow.json, etc. just TFA wants to be a unicorn and
split it up in files per user - seems rather inconsistent.
And, admins should resort to working on files directly as a last measurement, CLI
tools and the GUI should be preferred, *always*, so that's a moot point.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [pbs-devel] [RFC backup 0/6] Two factor authentication
2020-12-02 12:35 ` Oguz Bektas
2020-12-02 12:51 ` Wolfgang Bumiller
@ 2020-12-02 13:07 ` Thomas Lamprecht
2020-12-02 13:35 ` Oguz Bektas
1 sibling, 1 reply; 23+ messages in thread
From: Thomas Lamprecht @ 2020-12-02 13:07 UTC (permalink / raw)
To: Oguz Bektas; +Cc: Proxmox Backup Server development discussion
On 02.12.20 13:35, Oguz Bektas wrote:
> On Wed, Dec 02, 2020 at 01:27:47PM +0100, Thomas Lamprecht wrote:
>>> 3. don't store all the tfa information in a single json file.
>>>
>>
>> makes no sense to me, any reason you mention below can happen to arbitrary
>> files, so just adds complexity while not gaining anything.
>>
>>> current version uses a single /etc/proxmox-backup/tfa.json file
>>> which holds all the tfa info for all the users. this is a single point
>>> of failure because:
>>> - file can be corrupted, causing tfa to break for everyone (no more logins)
>>> - file could get deleted, disabling/bypassing 2fa for everyone
>>> - file could get leaked in a backup etc., giving everyone's tfa secrets
>>> and/or recovery keys to attackers (bypass everything)
>>>
>>> better is to at least create a file for each user:
>>> /etc/proxmox-backup/tfa/<username>.json or similar
>>>
>>> this way the damage is contained if for example the config breaks
>>> because of incorrect deserialization etc.
>>
>> Why would deserialisation be incorrect for one single file but magically
>> works if multiple files? Makes no sense.
>
> of course this can happen on arbitrary files... i don't see why it
> would add any complexity to use multiple files though (actually makes it
> simpler imo). the reasoning behind this was to avoid a single point of
> failure like i explained:
>
> multiple files for users -> only that user is affected by broken config,
> other users can log in
> single file for all users -> all users affected if config breaks and
> nobody can log in
See that almost as anti-feature, it's actually better if such a thing happens
that it's broken for all, as then one gets admin attention and can actually
look for the underlying root cause - which at that point is probably memory or
disk corruption/failure - or where does wolfgangs serializer breaks for all in one
but not for split??
>
> so the point wasn't to magically fix (potential) incorrect deserialization but to
> reduce breakage in case something like that happens.
like "what" happens? There's no such thing as one serialization is fine and the
other not - if you start assuming that transient error model you cannot do anything
at all anymore!
I rather have it corrupt for all files as then the admin needs to fix it and we
get notified, as some "magic" bug that only happens if it's a Tuesday and full moon.
So no I do *not* want to have user.cfg, token.cfg, shadow.json with all info in
one file, and then start to split TFA for every user, because of an error model
which just assumes whatever one wishes.
>>
>>> 5. notify user if more than X failed tfa attempts (password is already
>>> compromised at this point, so it's important to notify) and block IP
>>> for certain amount of time (fail2ban?)
>>
>> we do not setup fail2ban but any admin can already if wished. Notification
>> can only work if the user has setup a mail in the first place - but yes, sou
> yes, but imo 2fa is more sensitive to bruteforcing than regular
> passwords so it would make sense to limit it by default
why is it more sensitive? I need both, so it's the same? If I get leaked shadow
and tfa, I need to break both, only one has no use - that's the idea of TFA...
>>
>>>
>>> 5.b also if recovery keys are available, limit amount of TOTP attempts
>>> for that user
>>
>> what?
>>
>
> if a user sets up TOTP + recovery keys, then it would make sense to lock
> account in case of a lot of auth attempts with TOTP, until recovery key
> is entered (afaik this is a common mechanism). but maybe just
> notifying the user is enough as well.
and why do you place more trust onto the fixed recovery keys than another TFA
option? Which services/programs/websites do that, can you name a few examples?
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [pbs-devel] [RFC backup 0/6] Two factor authentication
2020-12-02 13:07 ` Thomas Lamprecht
@ 2020-12-02 13:35 ` Oguz Bektas
2020-12-02 14:05 ` Thomas Lamprecht
0 siblings, 1 reply; 23+ messages in thread
From: Oguz Bektas @ 2020-12-02 13:35 UTC (permalink / raw)
To: Thomas Lamprecht; +Cc: Proxmox Backup Server development discussion
On Wed, Dec 02, 2020 at 02:07:25PM +0100, Thomas Lamprecht wrote:
> On 02.12.20 13:35, Oguz Bektas wrote:
> > On Wed, Dec 02, 2020 at 01:27:47PM +0100, Thomas Lamprecht wrote:
> >>> 3. don't store all the tfa information in a single json file.
> >>>
> >>
> >> makes no sense to me, any reason you mention below can happen to arbitrary
> >> files, so just adds complexity while not gaining anything.
> >>
> >>> current version uses a single /etc/proxmox-backup/tfa.json file
> >>> which holds all the tfa info for all the users. this is a single point
> >>> of failure because:
> >>> - file can be corrupted, causing tfa to break for everyone (no more logins)
> >>> - file could get deleted, disabling/bypassing 2fa for everyone
> >>> - file could get leaked in a backup etc., giving everyone's tfa secrets
> >>> and/or recovery keys to attackers (bypass everything)
> >>>
> >>> better is to at least create a file for each user:
> >>> /etc/proxmox-backup/tfa/<username>.json or similar
> >>>
> >>> this way the damage is contained if for example the config breaks
> >>> because of incorrect deserialization etc.
> >>
> >> Why would deserialisation be incorrect for one single file but magically
> >> works if multiple files? Makes no sense.
> >
> > of course this can happen on arbitrary files... i don't see why it
> > would add any complexity to use multiple files though (actually makes it
> > simpler imo). the reasoning behind this was to avoid a single point of
> > failure like i explained:
> >
> > multiple files for users -> only that user is affected by broken config,
> > other users can log in
> > single file for all users -> all users affected if config breaks and
> > nobody can log in
>
> See that almost as anti-feature, it's actually better if such a thing happens
> that it's broken for all, as then one gets admin attention and can actually
> look for the underlying root cause - which at that point is probably memory or
> disk corruption/failure - or where does wolfgangs serializer breaks for all in one
> but not for split??
>
>
> >
> > so the point wasn't to magically fix (potential) incorrect deserialization but to
> > reduce breakage in case something like that happens.
>
>
> like "what" happens? There's no such thing as one serialization is fine and the
> other not - if you start assuming that transient error model you cannot do anything
> at all anymore!
as i explained already, it's not about if one serialization is fine and
the other isnt; if we have one big mess of a json file holding all the
secrets of everyone's tfa config, and at any point there's some bug in
the serializer or any other component that interacts with this, then
this can lead to DOS of ALL accounts on the server (or compromise of ALL
secrets in that file). the model is different than the normal authentication
mechanism with pam/pbs realms, since the 2fa configuration has
(untrusted) user input that gets serialized and added into a root-owned
file during the setup. letting any user on any realm do this is IMO
bad practice.
furthermore we could easily add a check during auth to see if the
tfa.json parses to correct json, and if not pop up an error message like
"2FA configuration invalid, please contact administrator" etc. and even
automatically send an email to root@pam ...
>
> I rather have it corrupt for all files as then the admin needs to fix it and we
> get notified, as some "magic" bug that only happens if it's a Tuesday and full moon.
>
> So no I do *not* want to have user.cfg, token.cfg, shadow.json with all info in
> one file, and then start to split TFA for every user, because of an error model
> which just assumes whatever one wishes.
>
> >>
> >>> 5. notify user if more than X failed tfa attempts (password is already
> >>> compromised at this point, so it's important to notify) and block IP
> >>> for certain amount of time (fail2ban?)
> >>
> >> we do not setup fail2ban but any admin can already if wished. Notification
> >> can only work if the user has setup a mail in the first place - but yes, sou
> > yes, but imo 2fa is more sensitive to bruteforcing than regular
> > passwords so it would make sense to limit it by default
>
> why is it more sensitive? I need both, so it's the same? If I get leaked shadow
> and tfa, I need to break both, only one has no use - that's the idea of TFA...
it's more sensitive to bruteforcing; because of limited
keyspace, as in it's easier to bruteforce a 6
digit numerical passcode than a regular passphrase in most
circumstances. if attacker cracks/steals a password and is presented
with a 2fa screen, it should be unlikely for them to bypass/crack that.
if i get unlimited tries to crack a 6-digit code you'll eventually get
it right.
that's why i think attempts should be limited by default and not reliant
on fail2ban, because there's no use case where anyone tries to enter a
totp code a thousand times for any legitimate reason... (however you
could forget/lose your password easily so it's more acceptable to let
someone keep trying in the regular auth case)
>
>
> >>
> >>>
> >>> 5.b also if recovery keys are available, limit amount of TOTP attempts
> >>> for that user
> >>
> >> what?
> >>
> >
> > if a user sets up TOTP + recovery keys, then it would make sense to lock
> > account in case of a lot of auth attempts with TOTP, until recovery key
> > is entered (afaik this is a common mechanism). but maybe just
> > notifying the user is enough as well.
>
> and why do you place more trust onto the fixed recovery keys than another TFA
> option?
the same reason i explained above, this would only kick in when the TOTP
is disabled because of too many auth failures. if a user has set up
recovery keys then they can be already used instead of TOTP (the option
is there regardless). so it's not placing more trust on the recovery
keys.
the flow could be something like this:
1. user sets up 2fa, TOTP and recovery keys
2. attacker login with stolen password
3. attacker attempts to crack 2fa totp code
4. fails after 3/5/X attempts, user gets notified and TOTP is disabled
5. at this point user can only log in with password + recovery code. (which they
could anyway, even if TOTP is present)
> Which services/programs/websites do that, can you name a few examples?
afaik some "secure" email providers like protonmail/tutanota etc. use
this kind of mechanism (account password + mailbox is encrypted with
password, and recovery keys in case all else is lost/locked).
i'm sure there are other examples as well
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [pbs-devel] [RFC backup 0/6] Two factor authentication
2020-12-02 13:35 ` Oguz Bektas
@ 2020-12-02 14:05 ` Thomas Lamprecht
2020-12-02 14:21 ` Oguz Bektas
0 siblings, 1 reply; 23+ messages in thread
From: Thomas Lamprecht @ 2020-12-02 14:05 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Oguz Bektas
On 02.12.20 14:35, Oguz Bektas wrote:
> On Wed, Dec 02, 2020 at 02:07:25PM +0100, Thomas Lamprecht wrote:
>> On 02.12.20 13:35, Oguz Bektas wrote:
>>> On Wed, Dec 02, 2020 at 01:27:47PM +0100, Thomas Lamprecht wrote:
>>>>> 3. don't store all the tfa information in a single json file.
>>>>>
>>>>
>>>> makes no sense to me, any reason you mention below can happen to arbitrary
>>>> files, so just adds complexity while not gaining anything.
>>>>
>>>>> current version uses a single /etc/proxmox-backup/tfa.json file
>>>>> which holds all the tfa info for all the users. this is a single point
>>>>> of failure because:
>>>>> - file can be corrupted, causing tfa to break for everyone (no more logins)
>>>>> - file could get deleted, disabling/bypassing 2fa for everyone
>>>>> - file could get leaked in a backup etc., giving everyone's tfa secrets
>>>>> and/or recovery keys to attackers (bypass everything)
>>>>>
>>>>> better is to at least create a file for each user:
>>>>> /etc/proxmox-backup/tfa/<username>.json or similar
>>>>>
>>>>> this way the damage is contained if for example the config breaks
>>>>> because of incorrect deserialization etc.
>>>>
>>>> Why would deserialisation be incorrect for one single file but magically
>>>> works if multiple files? Makes no sense.
>>>
>>> of course this can happen on arbitrary files... i don't see why it
>>> would add any complexity to use multiple files though (actually makes it
>>> simpler imo). the reasoning behind this was to avoid a single point of
>>> failure like i explained:
>>>
>>> multiple files for users -> only that user is affected by broken config,
>>> other users can log in
>>> single file for all users -> all users affected if config breaks and
>>> nobody can log in
>>
>> See that almost as anti-feature, it's actually better if such a thing happens
>> that it's broken for all, as then one gets admin attention and can actually
>> look for the underlying root cause - which at that point is probably memory or
>> disk corruption/failure - or where does wolfgangs serializer breaks for all in one
>> but not for split??
>>
>>
>>>
>>> so the point wasn't to magically fix (potential) incorrect deserialization but to
>>> reduce breakage in case something like that happens.
>>
>>
>> like "what" happens? There's no such thing as one serialization is fine and the
>> other not - if you start assuming that transient error model you cannot do anything
>> at all anymore!
>
> as i explained already, it's not about if one serialization is fine and
> the other isnt; if we have one big mess of a json file holding all the
> secrets of everyone's tfa config, and at any point there's some bug in
> the serializer or any other component that interacts with this, then
> this can lead to DOS of ALL accounts on the server (or compromise of ALL
> secrets in that file). the model is different than the normal authentication
> mechanism with pam/pbs realms, since the 2fa configuration has
> (untrusted) user input that gets serialized and added into a root-owned
> file during the setup. letting any user on any realm do this is IMO
> bad practice.
It's not a mess it's clearly structured. Serde does just a fine job serializing
JSON, a simple format to escape, plus we define schemas with validation for that
exact reason.
>
> furthermore we could easily add a check during auth to see if the
> tfa.json parses to correct json, and if not pop up an error message like
> "2FA configuration invalid, please contact administrator" etc. and even
> automatically send an email to root@pam ...
That's what serde already does, it errors if not valid JSON, which then erros
the login (did not looked at it, but would assume that a error there does not
just defuses TFA completely...) ...
>
>>
>> I rather have it corrupt for all files as then the admin needs to fix it and we
>> get notified, as some "magic" bug that only happens if it's a Tuesday and full moon.
>>
>> So no I do *not* want to have user.cfg, token.cfg, shadow.json with all info in
>> one file, and then start to split TFA for every user, because of an error model
>> which just assumes whatever one wishes.
>>
>>>>
>>>>> 5. notify user if more than X failed tfa attempts (password is already
>>>>> compromised at this point, so it's important to notify) and block IP
>>>>> for certain amount of time (fail2ban?)
>>>>
>>>> we do not setup fail2ban but any admin can already if wished. Notification
>>>> can only work if the user has setup a mail in the first place - but yes, sou
>>> yes, but imo 2fa is more sensitive to bruteforcing than regular
>>> passwords so it would make sense to limit it by default
>>
>> why is it more sensitive? I need both, so it's the same? If I get leaked shadow
>> and tfa, I need to break both, only one has no use - that's the idea of TFA...
>
> it's more sensitive to bruteforcing; because of limited
> keyspace, as in it's easier to bruteforce a 6
> digit numerical passcode than a regular passphrase in most
> circumstances. if attacker cracks/steals a password and is presented
> with a 2fa screen, it should be unlikely for them to bypass/crack that.
> if i get unlimited tries to crack a 6-digit code you'll eventually get
> it right.
You have about 2 time windows to get through all combinations of 10^6 with
a forced response delay of 3 seconds + network latency, so 20 tries max before
the time change so much that you need to start again...
>
> that's why i think attempts should be limited by default and not reliant
> on fail2ban, because there's no use case where anyone tries to enter a
> totp code a thousand times for any legitimate reason... (however you
> could forget/lose your password easily so it's more acceptable to let
> someone keep trying in the regular auth case)
but fail2ban can cope with the difference between >3 tries per minute, so
why exactly
>>>>
>>>>>
>>>>> 5.b also if recovery keys are available, limit amount of TOTP attempts
>>>>> for that user
>>>>
>>>> what?
>>>>
>>>
>>> if a user sets up TOTP + recovery keys, then it would make sense to lock
>>> account in case of a lot of auth attempts with TOTP, until recovery key
>>> is entered (afaik this is a common mechanism). but maybe just
>>> notifying the user is enough as well.
>>
>> and why do you place more trust onto the fixed recovery keys than another TFA
>> option?
> the same reason i explained above, this would only kick in when the TOTP
> is disabled because of too many auth failures. if a user has set up
> recovery keys then they can be already used instead of TOTP (the option
> is there regardless). so it's not placing more trust on the recovery
> keys.
It sure is, because you say that recovery keys are still good when u2f is not
anymore, that implies you trusting it more that u2f or other variants.
>
> the flow could be something like this:
> 1. user sets up 2fa, TOTP and recovery keys
> 2. attacker login with stolen password
> 3. attacker attempts to crack 2fa totp code
> 4. fails after 3/5/X attempts, user gets notified and TOTP is disabled
> 5. at this point user can only log in with password + recovery code. (which they
> could anyway, even if TOTP is present)
>
>> Which services/programs/websites do that, can you name a few examples?
>
> afaik some "secure" email providers like protonmail/tutanota etc. use
proton mail seems to use just 8 hex characters as recovery key and I see nowhere
any description for the behaviour you suggest..
https://protonmail.com/support/knowledge-base/two-factor-authentication/
> this kind of mechanism (account password + mailbox is encrypted with
> password, and recovery keys in case all else is lost/locked).
>
> i'm sure there are other examples as well
I'm then sure you can list them, for now we're at 0 examples with actual
source ;-)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [pbs-devel] [RFC backup 0/6] Two factor authentication
2020-12-02 14:05 ` Thomas Lamprecht
@ 2020-12-02 14:21 ` Oguz Bektas
2020-12-02 14:29 ` Wolfgang Bumiller
0 siblings, 1 reply; 23+ messages in thread
From: Oguz Bektas @ 2020-12-02 14:21 UTC (permalink / raw)
To: Thomas Lamprecht; +Cc: Proxmox Backup Server development discussion
On Wed, Dec 02, 2020 at 03:05:42PM +0100, Thomas Lamprecht wrote:
> On 02.12.20 14:35, Oguz Bektas wrote:
> > On Wed, Dec 02, 2020 at 02:07:25PM +0100, Thomas Lamprecht wrote:
> >> On 02.12.20 13:35, Oguz Bektas wrote:
> >>> On Wed, Dec 02, 2020 at 01:27:47PM +0100, Thomas Lamprecht wrote:
> >>>>> 3. don't store all the tfa information in a single json file.
> >>>>>
> >>>>
> >>>> makes no sense to me, any reason you mention below can happen to
> >>>> arbitrary files, so just adds complexity while not gaining
> >>>> anything.
> >>>>
> >>>>> current version uses a single /etc/proxmox-backup/tfa.json file
> >>>>> which holds all the tfa info for all the users. this is a single
> >>>>> point of failure because: - file can be corrupted, causing tfa
> >>>>> to break for everyone (no more logins) - file could get deleted,
> >>>>> disabling/bypassing 2fa for everyone - file could get leaked in
> >>>>> a backup etc., giving everyone's tfa secrets and/or recovery
> >>>>> keys to attackers (bypass everything)
> >>>>>
> >>>>> better is to at least create a file for each user:
> >>>>> /etc/proxmox-backup/tfa/<username>.json or similar
> >>>>>
> >>>>> this way the damage is contained if for example the config
> >>>>> breaks because of incorrect deserialization etc.
> >>>>
> >>>> Why would deserialisation be incorrect for one single file but
> >>>> magically works if multiple files? Makes no sense.
> >>>
> >>> of course this can happen on arbitrary files... i don't see why
> >>> it would add any complexity to use multiple files though (actually
> >>> makes it simpler imo). the reasoning behind this was to avoid a
> >>> single point of failure like i explained:
> >>>
> >>> multiple files for users -> only that user is affected by broken
> >>> config, other users can log in single file for all users -> all
> >>> users affected if config breaks and nobody can log in
> >>
> >> See that almost as anti-feature, it's actually better if such a
> >> thing happens that it's broken for all, as then one gets admin
> >> attention and can actually look for the underlying root cause -
> >> which at that point is probably memory or disk corruption/failure -
> >> or where does wolfgangs serializer breaks for all in one but not
> >> for split??
> >>
> >>
> >>>
> >>> so the point wasn't to magically fix (potential) incorrect
> >>> deserialization but to reduce breakage in case something like that
> >>> happens.
> >>
> >>
> >> like "what" happens? There's no such thing as one serialization is
> >> fine and the other not - if you start assuming that transient error
> >> model you cannot do anything at all anymore!
> >
> > as i explained already, it's not about if one serialization is fine
> > and the other isnt; if we have one big mess of a json file holding
> > all the secrets of everyone's tfa config, and at any point there's
> > some bug in the serializer or any other component that interacts
> > with this, then this can lead to DOS of ALL accounts on the server
> > (or compromise of ALL secrets in that file). the model is different
> > than the normal authentication mechanism with pam/pbs realms, since
> > the 2fa configuration has (untrusted) user input that gets
> > serialized and added into a root-owned file during the setup.
> > letting any user on any realm do this is IMO bad practice.
>
> It's not a mess it's clearly structured. Serde does just a fine job
> serializing JSON, a simple format to escape, plus we define schemas
> with validation for that exact reason.
>
> >
> > furthermore we could easily add a check during auth to see if the
> > tfa.json parses to correct json, and if not pop up an error message
> > like "2FA configuration invalid, please contact administrator" etc.
> > and even automatically send an email to root@pam ...
>
> That's what serde already does, it errors if not valid JSON, which
> then erros the login (did not looked at it, but would assume that a
> error there does not just defuses TFA completely...) ...
yes instead of erroring the login without any explanation we can do like
i suggested. that way we still get notified if something is wrong, and
without DOSing the whole server ;)
>
> >
> >>
> >> I rather have it corrupt for all files as then the admin needs to
> >> fix it and we get notified, as some "magic" bug that only happens
> >> if it's a Tuesday and full moon.
> >>
> >> So no I do *not* want to have user.cfg, token.cfg, shadow.json with
> >> all info in one file, and then start to split TFA for every user,
> >> because of an error model which just assumes whatever one wishes.
> >>
> >>>>
> >>>>> 5. notify user if more than X failed tfa attempts (password is
> >>>>> already compromised at this point, so it's important to notify)
> >>>>> and block IP for certain amount of time (fail2ban?)
> >>>>
> >>>> we do not setup fail2ban but any admin can already if wished.
> >>>> Notification can only work if the user has setup a mail in the
> >>>> first place - but yes, sou
> >>> yes, but imo 2fa is more sensitive to bruteforcing than regular
> >>> passwords so it would make sense to limit it by default
> >>
> >> why is it more sensitive? I need both, so it's the same? If I get
> >> leaked shadow and tfa, I need to break both, only one has no use -
> >> that's the idea of TFA...
> >
> > it's more sensitive to bruteforcing; because of limited keyspace, as
> > in it's easier to bruteforce a 6 digit numerical passcode than a
> > regular passphrase in most circumstances. if attacker cracks/steals
> > a password and is presented with a 2fa screen, it should be unlikely
> > for them to bypass/crack that. if i get unlimited tries to crack a
> > 6-digit code you'll eventually get it right.
>
> You have about 2 time windows to get through all combinations of 10^6
> with a forced response delay of 3 seconds + network latency, so 20
> tries max before the time change so much that you need to start
> again...
>
> >
> > that's why i think attempts should be limited by default and not
> > reliant on fail2ban, because there's no use case where anyone tries
> > to enter a totp code a thousand times for any legitimate reason...
> > (however you could forget/lose your password easily so it's more
> > acceptable to let someone keep trying in the regular auth case)
>
> but fail2ban can cope with the difference between >3 tries per minute,
> so why exactly
>
> >>>>
> >>>>>
> >>>>> 5.b also if recovery keys are available, limit amount of TOTP
> >>>>> attempts for that user
> >>>>
> >>>> what?
> >>>>
> >>>
> >>> if a user sets up TOTP + recovery keys, then it would make sense
> >>> to lock account in case of a lot of auth attempts with TOTP, until
> >>> recovery key is entered (afaik this is a common mechanism). but
> >>> maybe just notifying the user is enough as well.
> >>
> >> and why do you place more trust onto the fixed recovery keys than
> >> another TFA option?
> > the same reason i explained above, this would only kick in when the
> > TOTP is disabled because of too many auth failures. if a user has
> > set up recovery keys then they can be already used instead of TOTP
> > (the option is there regardless). so it's not placing more trust on
> > the recovery keys.
>
> It sure is, because you say that recovery keys are still good when u2f
> is not anymore, that implies you trusting it more that u2f or other
> variants.
no... that's just how recovery keys work (they are usable at *any* time
until used once). the decision to set up recovery key or not is up to
the user. this lockout mechanism would be only with TOTP setup enabled +
recovery keys also enabled (in case it wasn't clear in the previous
mails).
>
> >
> > the flow could be something like this:
> > 1. user sets up 2fa, TOTP and recovery keys
> > 2. attacker login with stolen password
> > 3. attacker attempts to crack 2fa totp code
> > 4. fails after 3/5/X attempts, user gets notified and TOTP is disabled
> > 5. at this point user can only log in with password + recovery code. (which they
> > could anyway, even if TOTP is present)
> >
> >> Which services/programs/websites do that, can you name a few examples?
> >
> > afaik some "secure" email providers like protonmail/tutanota etc. use
>
> proton mail seems to use just 8 hex characters as recovery key and I see nowhere
> any description for the behaviour you suggest..
>
> https://protonmail.com/support/knowledge-base/two-factor-authentication/
my bad then
>
> > this kind of mechanism (account password + mailbox is encrypted with
> > password, and recovery keys in case all else is lost/locked).
> >
> > i'm sure there are other examples as well
>
> I'm then sure you can list them, for now we're at 0 examples with actual
> source ;-)
here one example: https://docs.genesys.com/Documentation/GAAP/latest/iaHelp/2FA
"If the result is successful (i.e. the code is valid), Intelligent
Automation grants access. If the result is unsuccessful (i.e. the code
is invalid), Intelligent Automation prompts the user to enter another
code, until the Maximum Attempts at Sending an Authentication Code value
is reached."
so it is definitely a thing...
here is some explanation for the mathematics behind it: https://security.stackexchange.com/questions/185905/maximum-tries-for-2fa-code
if lockout isn't preferred, another solution would be for example to
increase the delay in a linear fashion after every failed 2fa auth attempt
(gets longer to auth for that IP address each time TOTP code failed).
however this can also be easily bypassed by using proxies etc. during
bruteforce so i'd prefer a lockout policy instead.
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [pbs-devel] [RFC backup 0/6] Two factor authentication
2020-12-02 14:21 ` Oguz Bektas
@ 2020-12-02 14:29 ` Wolfgang Bumiller
0 siblings, 0 replies; 23+ messages in thread
From: Wolfgang Bumiller @ 2020-12-02 14:29 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
> On 12/02/2020 3:21 PM Oguz Bektas <o.bektas@proxmox.com> wrote:
>
> if lockout isn't preferred, another solution would be for example to
> increase the delay in a linear fashion after every failed 2fa auth attempt
> (gets longer to auth for that IP address each time TOTP code failed).
> however this can also be easily bypassed by using proxies etc. during
> bruteforce so i'd prefer a lockout policy instead.
Personally I don't have a problem with locking second factors after
failed attempts because it will tell the user that their password has
most likely been compromised.
Regardless of all that though, this does not need to be subject of
this patch series. Eg. timeouts & lockouts are a general authentication
issue and should be a separate series. And even the TFA-specific ones
can be added afterwards, as that's just metadata and does not touch the
actual WA/TOTP/... implementation itself anyway.
So maybe we should switch the subject on this discussion or start
a new thread? ;-)
^ permalink raw reply [flat|nested] 23+ messages in thread