Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] redesign #13

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

[WIP] redesign #13

wants to merge 6 commits into from

Conversation

pzmarzly
Copy link
Member

Nareszcie uporządkowałem trochę napisany kod, więc wrzucam to co mam, żeby każdy mógł popatrzeć. To jest mój (wstępny) pomysł na nowe wnętrze projektu. Zamiast Unix socketa przyjmującego 1 połączenie na raz (i blokującego się na czas otwarcia drzwi), jest RESTowe API. Ponadto jest podział na klasy, jakieś proste testy, konfiguracja pipenv-a do zależności.

Code review w sensownej ilości mile widziany (nie jestem pythonowcem).

Do zrobienia przed zmerge'owaniem (chyba że chcemy mieć nowy branch np. next):

  • Przepisać adder.py i reader.py, tak żeby korzystały z API po HTTP, używając tokena z superuser_password.txt - powinno być super łatwe.
  • Dorobić prosty frontend [0]. Zdecydować, czy chcemy przydzielać jakieś hasła, czy może logowanie serial number-em legitymacji wystarczy (BTW dość nerdowe takie znanie swojego serial number karty, podoba mi się).
  • Zdecydować, czy chcemy pozwolić na wiele legitymacji przypiętych do 1 studenta. Jeśli tak, to usunąć unique constraint z SQLa, jeśli nie, to dodać API do usuwania legitymacji. Obecna wersja też ma ten problem, myślę, że to dobra okazja do naprawienia tego.
  • Pozmieniać deploy w folderze Infra - jedyna niezbędna zmiana jest taka, że zamiast worker.py będzie odpalany server.py, ale fajnie by było przygotować go na wystawienie do Internetu [1] przygotowując WSGI. Przy okazji część kodu będzie można usunąć (np. dependencies oddelegować pipenv-owi). Ten task na samym końcu.

[0] IMO wystarczy statyczny HTML z JSem z fetch API (bo przy logowaniu "czystym HTMLem" aka Basic Auth, przeglądarki słabo współpracują z menadżerami haseł).

let url = 'http://example.com/unlock';
let username = 'cokolwiek';
let password = serial_number;

let headers = new Headers();
headers.append('Content-Type', 'text/json');
headers.append('Authorization', 'Basic ' + btoa(username + ":" + password));

fetch(url, {method:'GET', headers: headers})
.then(response => response.json())
.then(json => console.log(json));

[1] Co do wystawienia serwera na świat, proponuję reverse SSH tunelling. Wtedy Pi łączyłby się z VPSem, a VPS dostając połączenie np. na port 5000 przekazywałby po SSH transmisję do Pi (oczywiście prostą konfiguracją nginxa można zrobić to ładniej). Z tego co pamiętam na domyślnych ustawieniach reverse proxy binduje się tylko do interfejsu local na VPSie (czyli nasz serwis z RPi byłby dostępny na VPSie tylko z localhosta z punktu widzenia VPSa, ale nie byłby dostępny z internetu). Trzeba zmienić jakąś linię w configu sshd żeby pozwolić na bindowanie do innych interfejsów, i potem z klienta łączyć się np. tak:

while true; do
   ssh -R :80:localhost:8000 -N [email protected]
   sleep 30
done

The goal is to make the program capable of handling concurrent events.

This eliminated the need for timeouts, which wouldn't really scale for
the future (we are planning to make a mobile site as an alternative
interface to the system).

Work in progress.
This way, we are prolonging door open time (but without stacking),
instead of cancelling concurrent events.

Old behavior: the door will be closed after n seconds since being
opened. Events that happened while the door was open were ignored.

New behavior: the door will be closed after n seconds since the last
event (e.g. the last card reader event).
@Bozydarek
Copy link
Member

Bardzo pobieżnie przejrzałem kod i mam kilka ogólnych uwag:

Czytając je pamiętaj, że to tylko i wyłącznie moje uwagi. Bardzo się cieszę, że ktoś chce ten projekt dalej rozwijać, a nie że za jakiś czas KSI wróci do używania klucza bo nikomu nie będzie się chciało czegoś zdebugować 😉

  • Niezależnie od tego co implementujesz i w jakim języku nigdy nie trzymaj i nie porównuj haseł otwartym tekstem
  • Sockety są znaczenie wydajniejsze i nie angażują nie potrzebnie stosu sieciowego co jest zupełnie zbyteczne kiedy komunikujesz procesy na jednym urządzeniu
  • Blokowanie się na czas otwarcia nie wynika z użycia socketów tylko tego jak jest teraz obsługiwane otwarcie zamka
  • Ale z pewnością dodanie API ułatwiłoby sterowanie tym z zewnątrz - ew. można spróbować użyć gRPC - generalnie początkowa idea polegała na tym że będziemy wysyłać do raspa komendy tak jak to się robi teraz kiedy się łączysz bezpośrenio na raspa
  • nie wgłębiałem się mocno w kod ale wygląda na to że będziesz co 0.1 sekundy angażował procesor do sprawdzania if self.lastUnlockRequestTime is not None oraz if time.time() - self.lastUnlockRequestTime > self.unlockTime - co moim zdaniem nie ma sensu (rozważ sytuacje gdzie nikt nie przychodzi do koła przez kilka dni) - nie lepiej idle'owć aż ktoś przyłoży kartę pn532.read_mifare()?
  • imo lepszym i prostszym rozwiązaniem jest użycie VPNa (np wireguard) niż SSH tunellingu
  • (bo przy logowaniu "czystym HTMLem" aka Basic Auth, przeglądarki słabo współpracują z menadżerami haseł) - nie do końca rozumiem co masz na myśli - Keepass nie ma z tym problemu 😜

@pzmarzly
Copy link
Member Author

Sockety są znaczenie wydajniejsze i nie angażują nie potrzebnie stosu sieciowego co jest zupełnie zbyteczne kiedy komunikujesz procesy na jednym urządzeniu

Słyszałem kiedyś, że ostatecznie chcemy mieć więcej sposobów na otwieranie zamka, np. przez appkę, stronę internetową, panel admina itd. Więc gdzieś ten interfejs HTTP będzie potrzebny. Faktycznie mamy 2 (albo więcej?) możliwości:

  • interfejs HTTP na RPi, VPS jako proxy
  • interfejs SSH na RPi, interfejs HTTP na VPSie (który będzie odpalał np. ssh drzwi-pi open-door) - zauważ że jeśli chodzi o użycie zasobów, ta opcja będzie prawdopodobniej bardziej zasobożerna z powodu negocjacji SSH (negocjacja szyfrowania wymagająca extra round tripa, DH, generowanie kluczy tymczasowych)

Blokowanie się na czas otwarcia nie wynika z użycia socketów tylko tego jak jest teraz obsługiwane otwarcie zamka

Chodziło mi o to, że obecny worker.py działa na 1 wątku blokująco, czyli kiedy on śpi (z powodu Door.open), to nie akceptuje połączeń (listener.accept()). A sama jego budowa (pętla while True) uniemożliwia mu przetwarzanie wielu połączeń na raz. Pewnie istnieją pythonowe biblioteki będące serwerami nasłuchującymi na unix socketach - ale jeśli mam stawiać serwer to już wolę serwer HTTP.

co 0.1 sekundy angażował procesor do sprawdzania ... nie lepiej idle'owć aż ktoś przyłoży kartę ..?

Też mi się ten spinner nie podoba, mimo że dla RPi odpalenie czegoś 10 razy na sekundę to nic (wątpię że będzie to jakkolwiek zauważalne, na moim PC ten wątek "zużywa" wg topa 0.0% CPU, przypuszczam że na RPi będzie tak samo). Gdybym miał w Pythonie coś na wzór Rust-owego kanału mpsc, to bym zrobił kolejkę wiadomości między wątkami, każda wiadomość zawierała by timestamp. Te kanały mpsc mają możliwość zarówno blocking read, jak i non-blocking read. Wtedy wątek odblokowujący drzwi by się blokował w oczekiwaniu na wiadomość, jednocześnie przed zamknięciem drzwi robiłby non-blocking read żeby sprawdzić czy nie chcemy "przedłużać" czasu otwarcia (bo następna osoba chce wejść, bo ktoś nadal trzyma legitymację przy czytniku itd). Ale nie znam niczego takiego, gdy po 5 minutach niczego nie znalazłem to stwierdziłem że kiedy indziej można to zoptymalizować.

imo lepszym i prostszym rozwiązaniem jest użycie VPNa (np wireguard) niż SSH tunellingu

W tym modelu gdzie VPS wysyła polecenia po SSH do RPi - pewnie tak. W tym modelu gdzie chcemy mieć przekierowanie portów - nie wiem czy to by cokolwiek nam uprościło. Wtedy chyba potrzebowalibyśmy nginxa jako proxy (tzn i tak fajnie by było go mieć, ale w modelu z tunnelingiem nie jest niezbędny). Choć z drugiej strony gdybyśmy robili SSH tunneling, to pewnie chcielibyśmy nowego usera na VPSie dla bezpieczeństwa, z /bin/false jako shell. Więc mi to obojętne, może znajdzie się ktoś kto lubi się bawić w deploye, wtedy niech on zdecyduje co woli.

nie do końca rozumiem co masz na myśli - Keepass nie ma z tym problemu

Kiedy natywny popup z pytaniem o hasło się pojawia, nie da się odpalać menadżerów haseł działających jako rozszerzenia do przeglądarek:

Screenshot from 2020-04-14 15-42-45

Niezależnie od tego co implementujesz i w jakim języku nigdy nie trzymaj i nie porównuj haseł otwartym tekstem

Ten superuser_password miał być używany tylko z localhosta, wiec traktowałem to jako taki .env. Dużo lepsze byłoby po prostu sprawdzenie origin, trzeba by napisać taki middleware do Flaska.

Za to jeśli chodzi o konta użytkowników, to faktycznie przy 8 znakowym serial number karty jest możliwy brute force, a wymiana legitymacji jest droga i czasochłonna. Nie chciałem jednak zmieniać formatu bazy danych jaki mamy (może niesłusznie, w końcu jest jeszcze ten problem z UNIQUE). Musimy znać te serial numbers na backendzie w plaintext (chyba że chcemy czekać na hashowanie po przyłożeniu legitymacji do czytnika kart), więc może jednak logowanie się nr-em legitymacji to nie taki dobry pomysł. Możemy więc albo założyć zwykłe hasła (najbezpieczniejsza opcja), albo zrobić logowanie hashem nr-u (wtedy przynajmniej znika możliwość brute force'a). W tej drugiej opcji chodzi mi o to, że serwer ma swój sekret, użytkownicy drzwi dostają jedynie bytes_to_human_readable(scrypt(serial + server_secret, nonce = server_secret)) i tym się logują, a serwer sobie to rekalkuluje i porównuje.

Chyba że ci chodziło o HTTP vs HTTPS. Nie wiem, jak wygląda VPS KSI, ale strona KSI stoi na HTTPSie, a komunikacja VPS - RPi szła by po Wireguard albo po SSH, więc też nie plaintextem.

Tak czy inaczej, ja w najbliższej przyszłości raczej nie będę się skupiał na drzwiach otwartych, dlatego starałem się w moich wiadomościach opisać wszystkie pomysły i problemy jakie mam. Jeśli ktoś chce napisać jakiś kod, i przez to podejmie jakąś decyzję dotyczącą tego redesignu za mnie, to nie mam nic przeciwko.

@Bozydarek
Copy link
Member

Wow, sporo tekstu - tak na szybko w keepasie możesz w ustawieniach włączyć automatyczne wypełnianie HTTP Basic Auth - u mnie działa 😉 Resztę tekstu przeczytam z większym zrozumieniem jak tylko znajdę chwilę :)

@pzmarzly pzmarzly mentioned this pull request Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants