Skip to content

Refactor: auth::Key#197

Merged
josecelano merged 2 commits intotorrust:developfrom
josecelano:refactor-auth-key
Feb 28, 2023
Merged

Refactor: auth::Key#197
josecelano merged 2 commits intotorrust:developfrom
josecelano:refactor-auth-key

Conversation

@josecelano
Copy link
Copy Markdown
Member

@josecelano josecelano commented Feb 27, 2023

The struct KeyId was extracted to wrap the primitive type, but it was not being used in the auth::Key struct yet.

Key before this PR:

pub struct Key {
    pub key: String,
    pub valid_until: Option<DurationSinceUnixEpoch>,
}

After this PR:

pub struct ExpiringKey {
    pub id: KeyId,
    pub valid_until: Option<DurationSinceUnixEpoch>,
}
  • Use the new struct auth::KeyIdin the auth::Key struct.
  • Rename auth::Key to auth::ExpiringKey.

The struct `KeyId` was extracted to wrap the primitive type but it was not
being used in the `auth::Key` struct.
@josecelano josecelano linked an issue Feb 27, 2023 that may be closed by this pull request
@josecelano
Copy link
Copy Markdown
Member Author

josecelano commented Feb 27, 2023

Hi @da2ce7 @WarmBeer, I want to decouple authentication (the Key is valid) from authorization (the peer can get the whitelisted torrent) in the new HTTP Axum implementation.

The authenticated_request function has both responsibilities (or authentication inside the authorization):

pub async fn authenticate_request(&self, info_hash: &InfoHash, key: &Option<KeyId>) -> Result<(), Error> {
    // no authentication needed in public mode
    if self.is_public() {
        return Ok(());
    }

    // check if auth_key is set and valid
    if self.is_private() {
        match key {
            Some(key) => {
                if let Err(e) = self.verify_auth_key(key).await {
                    return Err(Error::PeerKeyNotValid {
                        key_id: key.clone(),
                        source: (Arc::new(e) as Arc<dyn std::error::Error + Send + Sync>).into(),
                    });
                }
            }
            None => {
                return Err(Error::PeerNotAuthenticated {
                    location: Location::caller(),
                });
            }
        }
    }

    // check if info_hash is whitelisted
    if self.is_whitelisted() && !self.is_info_hash_whitelisted(info_hash).await {
        return Err(Error::TorrentNotWhitelisted {
            info_hash: *info_hash,
            location: Location::caller(),
        });
    }

    Ok(())
}

That's fine, but I want to use directly the verify_auth_key in the HTTP announce handlers to check if the peer is authenticated when the tracker is running in private mode. I can do it since the function is public, but I wanted to finish this refactor in order to use a KeyId which does not require you to set a zero expiration time. In this context, the key expiration time does not make any sense.

pub async fn verify_auth_key(&self, key_id: &KeyId) -> Result<(), auth::Error> {
    match self.keys.read().await.get(key_id) {
        None => Err(auth::Error::UnableToReadKey {
            location: Location::caller(),
            key_id: Box::new(key_id.clone()),
        }),
        Some(key) => auth::verify(key),
    }
}

This was a pending refactor to use the KeyIdin the Key and rename the Key struct.

The refactor is ready to review.

@josecelano josecelano marked this pull request as ready for review February 27, 2023 19:12
@josecelano josecelano merged commit 2c4c32f into torrust:develop Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Refactor proposal: rename auth key structs

1 participant