Implementação de duplo fator de autenticação TOTP + códigos de recuperação (backend) #1769
Conversation
|
@lspaulucio is attempting to deploy a commit to the TabNews Team on Vercel. A member of the Team first needs to authorize it. |
|
Essa PR é basicamente a mesma que a #1709. |
Rafatcb
added
back
There was a problem hiding this comment.
Show, @lspaulucio! Não revisei o código, essa minha revisão é apenas sobre as alterações necessárias na interface da API e do banco de dados.
Aproveito para mencionar que editei o texto do seu PR para não fecharmos o issue #1171 após o merge, porque este PR contém apenas a parte do backend.
- Vamos mudar para que a ativação e desativação do TOTP seja feito no mesmo endpoint?
Me parece uma boa simplificação. Refleti sobre o que discutimos no outro PR e sobre o que é necessário para implementar esse novo recurso. Acredito que os endpoints envolvidos são algo como:
GET /api/v1/mfa/totp: Retorna a string necessária para o cliente gerar o código QR, e o usuário gerar e salvar o TOTP com um aplicativo.PATCH /api/v1/users/[username]: Envio dototp_secretetotp_token. Responsável por adicionar ou remover o TOTP à conta do usuário.- Se
totp_secretestiver definido, verifica se o usuário não possuitotp_secret, e se ototp_tokenenviado é válido. Se sim, atualiza o usuário com ototp_secretinformado. - Se
totp_secretfornull, verifica ototp_tokenenviado e ototp_secretatual do usuário, se for válido, salva ototp_token = NULL.
- Se
POST /api/v1/sessions: Caso o usuário tenha umtotp_secretsalvo no banco de dados, é necessário o envio dototp_tokenpara que seja validado.
Com isso, podemos remover também o POST /api/v1/mfa/totp/verify, porque existem apenas dois momentos onde é necessário verificar o código:
- Ao criar um novo código para a conta, e isso seria verificado no
PATCH /api/v1/users/[username]. - Ao acessar a conta ou realizar outra operação que necessite a segunda camada de verificação. Atualmente, isso seria realizado no
POST /api/v1/sessions.
Faz sentido a API ser dessa forma? É possível realizar todas as operações necessárias ou eu esqueci de algum detalhe? Me corrijam se eu estiver errado.
Não tenho certeza se alterar o PATCH /api/v1/users/[username] é melhor do que criar um novo endpoint para isso, visto que limitaria nossas opções para adicionar uma confirmação extra ao retornar códigos de recuperação para o usuário (como o próprio GitHub e outros sites), mas exigir essa confirmação no futuro seria uma breaking change. Imagens de como é no GitHub:
- Geração do código QR.
- Códigos de recuperação (ainda é possível cancelar a operação).
- Finalização.
- Vamos manter o booleano ou vamos deixar apenas o secret para indicar se o usuário está com o TOTP habilitado ou não?
Acho que não tem necessidade de manter o totp_enabled, podemos verificar isso pelo totp_secret 👍
SELECT totp_enabled IS NOT NULL AS totp_enabled FROM users;@aprendendofelipe você pode revisar aqui também? Assim o @lspaulucio não implementa algo sem necessidade, caso você tenha uma visão diferente.
|
Oi @Rafatcb, obrigado pelos comentários! Vou tentar colocar aqui abaixo o que eu havia pensado/implementado inicialmente, talvez possa facilitar o entendimento de quem for ajudar na revisão. Inicialmente, havia pensado nesses endpoints aqui:
Mas pensando agora, e vendo também o que você havia falado aqui:
Acho que poderíamos simplificar tudo isso em um único endpoint
Com relação aos pontos que você trouxe:
Eu havia pensado dessa forma mesmo, realizei os ajustes nesse endpoint para realizar essa verificação.
Também não gosto da ideia de usar esse endpoint Com relação aos códigos de recuperação, eu fiz um pouco diferente 😅 (não sei se foi a melhor forma 😄) |
Com um único endpoint a semântica não fica correta, porque o
Acho que adicionarmos os códigos de recuperação agora pode ser uma complicação sem necessidade, atrasando o PR por causa do esforço extra necessário. Conheço serviços grandes que, caso você queira trocar de aplicativo autenticador, precisa entrar em contato com o suporte... Não é uma boa referência, mas só estou mencionando porque dá para funcionar assim, pelo menos a princípio.
Me parece que isso tiraria o sentido do TOTP. Imagine que o invasor tem acesso ao e-mail, mas não ao token. Nesse caso, ele conseguiria burlar a proteção do TOTP. |
There was a problem hiding this comment.
- Vamos mudar para que a ativação e desativação do TOTP seja feito no mesmo endpoint?
Me parece uma boa simplificação. Refleti sobre o que discutimos no outro PR e sobre o que é necessário para implementar esse novo recurso. Acredito que os endpoints envolvidos são algo como:
GET /api/v1/mfa/totp: Retorna a string necessária para o cliente gerar o código QR, e o usuário gerar e salvar o TOTP com um aplicativo.
PATCH /api/v1/users/[username]: Envio dototp_secretetotp_token. Responsável por adicionar ou remover o TOTP à conta do usuário.
- Se
totp_secretestiver definido, verifica se o usuário não possuitotp_secret, e se ototp_tokenenviado é válido. Se sim, atualiza o usuário com ototp_secretinformado.- Se
totp_secretfornull, verifica ototp_tokenenviado e ototp_secretatual do usuário, se for válido, salva ototp_token = NULL.
POST /api/v1/sessions: Caso o usuário tenha umtotp_secretsalvo no banco de dados, é necessário o envio dototp_tokenpara que seja validado.Com isso, podemos remover também o
POST /api/v1/mfa/totp/verify, porque existem apenas dois momentos onde é necessário verificar o código:
- Ao criar um novo código para a conta, e isso seria verificado no
PATCH /api/v1/users/[username].- Ao acessar a conta ou realizar outra operação que necessite a segunda camada de verificação. Atualmente, isso seria realizado no
POST /api/v1/sessions.Faz sentido a API ser dessa forma? É possível realizar todas as operações necessárias ou eu esqueci de algum detalhe? Me corrijam se eu estiver errado.
Faz todo sentido ser exatamente assim 👍
Não tenho certeza se alterar o
PATCH /api/v1/users/[username]é melhor do que criar um novo endpoint para isso, visto que limitaria nossas opções para adicionar uma confirmação extra ao retornar códigos de recuperação para o usuário (como o próprio GitHub e outros sites), mas exigir essa confirmação no futuro seria uma breaking change.
Acho que o PATCH /api/v1/users/[username] é a melhor opção para o momento. E cuidando para isso só ser possível com update:user e não com update:user:others. Isso é feito pelo authorization.filterInput, onde apenas no update:user irá validar o totp_secret.
Questões extras para pensarmos são:
- Se o usuário estiver com o TOTP habilitado, vamos exigir o
totp_tokenpara qualquer modificação no cadastro do usuário, ou apenas para algumas? Quais? Deve ser mais simples pedir para tudo. - No caso da moderação usando a
update:user:others, devemos pedir ototp_tokendo moderador ou não é necessário? Acho melhor pedir, mas isso depende também da resposta para a pergunta acima.
- Vamos manter o booleano ou vamos deixar apenas o secret para indicar se o usuário está com o TOTP habilitado ou não?
Acho que não tem necessidade de manter o
totp_enabled...
Concordo que o booleano não é necessário! 👍
Como eu havia falado na issue #1171, na minha implementação eu não utilizei códigos de recuperação para esses casos (na época nem havia pensado nisso 😂 )
Acho que adicionarmos os códigos de recuperação agora pode ser uma complicação sem necessidade, atrasando o PR por causa do esforço extra necessário. Conheço serviços grandes que, caso você queira trocar de aplicativo autenticador, precisa entrar em contato com o suporte... Não é uma boa referência, mas só estou mencionando porque dá para funcionar assim, pelo menos a princípio.
Dá para funcionar assim, mas não vale a pena, pois teríamos que confiar unicamente no email enviado pelo usuário pedindo a recuperação, o que não seria nada mais seguro do que o dito abaixo. Por isso eu evitaria adicionar esse trabalho manual.
O que fiz para esse caso foi uma alteração no fluxo de recuperação de conta, de forma que caso um usuário venha a perder o token, basta ele realizar o fluxo normal de recuperação que o TOTP é removido, e ai se ele desejar basta configurar novamente um novo TOTP.
Me parece que isso tiraria o sentido do TOTP. Imagine que o invasor tem acesso ao e-mail, mas não ao token. Nesse caso, ele conseguiria burlar a proteção do TOTP.
Para melhorar isso, acho bom pedir a confirmação da senha junto ao token enviado pelo email. Assim como não deveria ser possível recuperar a senha sem fornecer o totp_token, caso esteja habilitado.
Show, já vou iniciar os ajustes para a remoção do booleano.
Certo, vou fazer os ajustes para que a habilitação do TOTP seja feito nesse endpoint.
Perfeito, concordo com vocês, na época não havia pensado nessa questão. Achei essa ideia de solicitar a confirmação da senha junto do token enviado por email ou de fornecer o
Vou dar uma pesquisada sobre esses códigos de recuperação para ver se é muito complexo implementar. Talvez o esforço necessário não seja tão alto assim para o benefício que vai trazer.
Sei que isso varia de sistema para sistema, existem os que:
Para mim, pensando do ponto de vista do usuário, um sistema que solicita o TOTP para qualquer alteração pode acabar atrapalhando a experiência. Porém, para o TabNews eu acho que não iria ficar tão ruim pois podemos solicitar apenas na hora que o usuário for salvar as alterações. Futuramente, se a quantidade de opções de alterações for muito grande podemos alterar para solicitar confirmação apenas para alguns campos... Mas para esse primeiro momento acho que podemos solicitar apenas na autenticação do usuário mesmo para simplificar, o que acham? |
Isso.
Não é difícil não. São como se fossem senhas que podem ser usadas uma única vez. Eu nunca usei, então não tenho certeza como faz para gerar novas, caso você use todas. Acho que é válido ler os FAQs de outras plataformas para termos um comportamento similar.
Eu acredito que devemos pedir apenas para alterar nome de usuário, senha e e-mail, pois são os dados mais sensíveis/importantes. Temos um usuário (marlon) que utiliza a API para atualizar a descrição todo dia, e exigir o TOTP para isso seria uma complicação desnecessária (além de possivelmente quebrar a integração sem ele perceber, caso ative o TOTP).
Faz sentido pedir quando um moderador edita, sim. Também fará sentido pedir no nuke e talvez em outras ações. Creio que podemos simplificar inicialmente. Não me parece que seria complicado adaptar o código da API para isso, mas a UI pode ser um pouco mais trabalhosa. Acredito que se esse PR incluir a funcionalidade do TOTP, a exigência dele no login, e os códigos de recuperação, já estará pronto para mesclar, e podemos tratar os outros pontos em outro PR (exigência do TOTP na edição, nuke etc.). |
|
@Rafatcb e @aprendendofelipe fiquei com algumas dúvidas nos pontos abaixo:
A grande maioria dos sistemas que implementam essa funcionalidade só oferecem a opção de códigos de recuperação após o usuário digitar o login e senha, pois a validação do TOTP é feita após essa etapa. Dessa forma, havia pensado em criar o endpoint Além disso, conforme o @aprendendofelipe havia comentado aqui:
No endpoint
Pelo que verifiquei alguns sistemas só permitem a visualização desses códigos uma única vez na configuração do TOTP. Já outros como o GitHub permitem que o usuário visualize os códigos cadastrados a qualquer momento nas configurações da conta. |
Eu acabei de fazer um teste no Discord.
Eu gostei dessas opções, e acho que podemos simplificar.
Não entendi o que é "recuperação do TOTP". Se a pessoa perdeu o TOTP, pode usar os códigos de recuperação. Se não tem acesso aos códigos, usa o TOTP. E uma vez que ela tenha acesso à algum dos dois, ela consegue desabilitar o TOTP.
No
Como comentei ali em cima, acho que por ora podemos seguir sem permitir visualizar novamente, porque existe uma forma do usuário contornar essa necessidade, realizando de outra forma. Meus comentários fazem sentido? Ficou com alguma dúvida? |
lspaulucio
force-pushed
the
feat/mfa-totp
branch
3 times, most recently
from
6a1f359 to
223c83f
Compare
|
Oi @Rafatcb Finalizei a implementação do TOTP com os códigos de recuperação 😀 No Vi que a |
Ainda não vi as alterações feitas, mas está com arquivos com conflitos, então fazer o |
|
Show @lspaulucio! Para facilitar a revisão, além de atualizar a branch, seria bom remover do PR qualquer alteração que não seja essencial para o TOTP. Isso inclui evitar refatorações ou mudanças de estilo em partes do código que não estão diretamente relacionadas ao TOTP, como adicionar ou remover linhas em branco, por exemplo. Melhorias em mensagens de erro e outros ajustes podem ser feitas em um PR específico. Em relação aos códigos de recuperação, vocês não acham que é uma funcionalidade diferente, e que merece um PR próprio? Mesmo que decidam manter tudo no mesmo PR, acho que é fundamental separar o código e os commits. Além disso, acho importante documentar (pode ser na descrição do PR) as etapas necessárias para a implantação dessa funcionalidade, tanto em homologação quanto em produção. O que precisa ser feito antes de mesclar o código e o que deve ser feito depois? Talvez seja necessário mais de um PR, como mesclar uma parte primeiro, rodar migrations, adicionar variáveis de ambiente, e depois mesclar o restante. Por fim, se já estiver tudo funcional e testado, seria legal você mesmo fazer uma revisão prévia. Isso ajuda a eliminar mudanças que não são essenciais e a melhorar a qualidade geral, como eliminar aqueles "ifs hadouken". |
|
Beleza! |
lspaulucio
force-pushed
the
feat/mfa-totp
branch
3 times, most recently
from
0818dbe to
d6c9ba0
Compare
lspaulucio
changed the title
|
Fala pessoal! Finalizei as alterações 🙂 Se vocês acharem que fica melhor quebrar essa PR em outras para facilitar me avisem, que ai faço a criação de outros PRs 😁 |
lspaulucio
force-pushed
the
feat/mfa-totp
branch
2 times, most recently
from
8a3c43f to
8642e62
Compare
lspaulucio
force-pushed
the
feat/mfa-totp
branch
from
8642e62 to
9f02ecf
Compare
There was a problem hiding this comment.
@lspaulucio, mais uma vez, muito obrigado por este PR! Sinto que ele está muito bem encaminhado, e a culpa dele estar aberto a tanto tempo não é sua. Estamos vendo outras coisas e ficamos sem tempo para revisar o PR aqui. Se você não possui tempo para continuar com esse PR, sem problemas, outra pessoa pode dar continuidade depois.
Falando por mim, estou com dificuldade em revisar e até deixei algumas dúvidas na revisão. Se você possui algum link de onde se inspirou para as decisões de implementação, pode compartilhar porque talvez me ajude a revisar.
Eu não consegui revisar tudo, e não precisa implementar o que eu revisei, porque talvez outro revisor discorde, a não ser que você já queira resolver os comentários mais simples que deixei (como a mudança em uma condicional, o try/catch, caso realmente não seja usado, e o teste que mencionei new URL()).
| pgm.addColumns('users', { | ||
| totp_recovery_codes: { | ||
| type: 'varchar(416)', | ||
| notNull: false, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Acho melhor isso ficar numa tabela separada, não sei se concordarão comigo.
Numa tabela própria, onde cada linha é um código associado à um usuário e com a indicação se o código já foi usado ou não.
| @@ -162,6 +166,7 @@ function filterOutput(user, feature, output) { | |||
| features: output.features, | |||
| tabcoins: output.tabcoins, | |||
| tabcash: output.tabcash, | |||
| totp_enabled: output.totp_secret !== null ? true : false, | |||
There was a problem hiding this comment.
Pequena simplificação:
| totp_enabled: output.totp_secret !== null ? true : false, | |
| totp_enabled: output.totp_secret !== null, |
| return totp.validate({ token }) !== null; | ||
| } | ||
|
|
||
| function makeCode(length = 10) { |
There was a problem hiding this comment.
Analisando o código, vejo dois grandes blocos de comportamento no models/otp:
- TOTP.
- Códigos de recuperação.
Sugiro deixarmos ambos em modelos separados, porque além de um não depender do outro, o TOTP não depende de nada do projeto, mas os códigos de recuperação utilizam o models/user. Se formos usar uma tabela separada para os códigos de recuperação, então, faz ainda mais sentido ter um modelo próprio.
Pensando mais sobre o assunto, me parece que devemos passar o código dos "códigos de recuperação" para junto do models/recovery. Ambos estão relacionados à recuperação de acesso à conta, e ter ou não TOTP influencia em como o acesso deve ser recuperado.
O que vocês acham?
There was a problem hiding this comment.
Se der para deixar os códigos de recuperação em um model separado, é melhor
| function encryptData(data) { | ||
| const iv = crypto.randomBytes(16); | ||
| const cipher = crypto.createCipheriv(cryptoConfigurations.algorithm, cryptoConfigurations.key, iv); | ||
| const encryptedData = Buffer.concat([cipher.update(data, 'utf8'), cipher.final()]); | ||
|
|
||
| return Buffer.concat([iv, encryptedData]).toString('hex'); | ||
| } | ||
|
|
||
| function decryptData(encrypted) { | ||
| const data = Buffer.from(encrypted, 'hex'); | ||
| const iv = data.subarray(0, 16); | ||
| const encryptedData = data.subarray(16); | ||
| const decipher = crypto.createDecipheriv(cryptoConfigurations.algorithm, cryptoConfigurations.key, iv); | ||
|
|
||
| return decipher.update(encryptedData, 'binary', 'utf-8') + decipher.final('utf-8'); | ||
| } |
There was a problem hiding this comment.
Essas duas funções podem ser generalizadas para um módulo separado? Ou esse processo é muito específico do TOTP?
Estou com certa dificuldade em entender as etapas necessárias para a criptografia, visto que não tenho tanta familiaridade com o assunto. Você obteve viu esse código (ou algo parecido) em um artigo ou documentação? Se sim, por favor, compartilhe o link caso encontrar.
| } | ||
|
|
||
| function validateUserTotp(userEncryptedSecret, token) { | ||
| const userSecret = decryptData(userEncryptedSecret); |
There was a problem hiding this comment.
Um detalhe: apesar do encryptData e decryptData estarem presentes em models/otp, em nenhum momento ele chama a função encryptData, então não vejo sentido em chamar o decryptData aqui.
Me parece que essa responsabilidade deveria ser de quem encriptou, e assim o models/otp trabalha diretamente com a string não-criptografada.
| } | ||
|
|
||
| async function validateAndMarkRecoveryCode(targetUser, recoveryCode) { | ||
| const recoveryCodes = JSON.parse(decryptData(targetUser.totp_recovery_codes)); |
There was a problem hiding this comment.
O mesmo que meu último comentário sobre decryptData.
| @@ -16,3 +16,5 @@ EMAIL_HTTP_PORT=1080 | |||
| EMAIL_USER= | |||
| EMAIL_PASSWORD= | |||
| UNDER_MAINTENANCE={"methodsAndPaths":["POST /api/v1/under-maintenance-test$"]} | |||
| TOTP_SECRET_KEY="ha0wFlJJXdeKwANNgoB8jT2Op1iQ0pfGWuqnK8oMTVg=" | |||
| TOTP_ENCRYPTION_METHOD="aes-256-cbc" | |||
There was a problem hiding this comment.
Como já disse, não entendo muito de criptografia. Pode me dizer porque essa foi a opção escolhida? Talvez seja óbvio, mas eu realmente não sei. Vi alguns lugares dizendo que aes-256-gcm pode ser melhor, mas não estudei a fundo para ver se faz sentido no nosso cenário.
| try { | ||
| response.status(200).json({ totp: totp.toString() }); | ||
| } catch (err) { | ||
| throw new ServiceError({ |
There was a problem hiding this comment.
Esse trecho de código lançou um erro em alguma situação?
| expect(responseBody).toStrictEqual({ | ||
| totp: `otpauth://totp/TabNews:${username}?issuer=TabNews&${secret}&algorithm=SHA1&digits=6&period=30`, | ||
| }); | ||
| expect(secret).toHaveLength(39); |
There was a problem hiding this comment.
Uma pequena melhoria nesse teste: ao invés de verificar o length=39, separar o secret= e validar diretamente o tamanho do segredo.
Provavelmente é mais fácil obter o parâmetro usando new URL() do que split..
Oi @Rafatcb, sem problemas, eu lembro que você já havia comentado que estavam priorizando outras funcionalidades mais urgentes 😃
Seguem abaixo os principais links que dei uma lida quando estava realizando a implementação: Melhores práticas: OTP:
Criptografia:
Beleza, essa semana eu não devo conseguir mexer porque estou com umas outras atividades pra fazer, mas acho que na semana que vem já consigo dar uma olhada nos comentários/sugestões que vocês colocaram, ai os casos mais simples já vou ajustando conforme a sugestão de vocês. Acredito que algumas coisas talvez não tenham ficado implementadas da melhor forma 😅, mas ai nas revisões a gente vai discutindo e eu vou adequando/refazendo de acordo com as sugestões de vocês. |
|
@lspaulucio Obrigado pela resposta rápida! Se eu conseguir um tempo para ler os links antes de você iniciar as alterações, talvez eu consiga também continuar a revisão, ou então já resolver algum comentário meu de dúvida. Quando eu continuar a revisão, marco este comentário como outdated para não poluir a thread. |
lspaulucio
force-pushed
the
feat/mfa-totp
branch
from
9f02ecf to
f49bc86
Compare
|
Oi pessoal! Dei uma olhada nos comentários que o @Rafatcb fez e acho que para facilitar a revisão de vocês (e as minhas correções também 😅) o melhor seria quebrar o PR em dois, igual foi sugerido pelo @aprendendofelipe nesse comentário aqui. Só fico com receio de que as sugestões já feitas para a parte dos códigos de recuperação se percam. Mas qualquer coisa eu faço uma cópia delas e incluo lá no novo PR. O que acham? Posso seguir dessa forma? |
Mudanças realizadas
Essa PR traz a implementação do backend para configuração do duplo fator de autenticação TOTP (Time-based one-time password).
O TOTP é um tipo de MFA (Múltiplo Fator de Autenticação). Ele se baseia na geração de um token ou código de acesso exclusivos baseado no tempo.
Dessa forma, ao se configurar o TOTP no momento do login além de e-mail e senha será solicitado o token de acesso.
A utilização de um outro fator de autenticação traz benefícios de segurança para o usuário uma vez que se sua conta for hackeada o invasor ainda precisará informar o token de acesso para que seja possível acessar a conta. Vários sites já implementam esse tipo de MFA, como por exemplo o próprio GitHub.
Também foi incluída nessa PR a implementação do backend para a utilização de códigos de recuperação, que podem ser usados para acessar a conta nas situações onde o usuário tenha perdido o acesso ao TOTP.
Endpoints Afetados
totpna tabela de usuário:totp_secret: Armazena a secret TOTP criptografadatotp_recovery_codes: Armazena os códigos de recuperação criptografadosotpauthpara implementação do TOTP/api/v1/mfa/totp.envpara criptografia do TOTPotp.jsauthorization.js,user.jsevalidator.jstotp, caso tenha sido habilitado, no endpoint/api/v1/recovery/api/v1/sessionpara inclusão de verificação TOTPtests/integration/api/v1/mfa/totptests/integration/api/v1/recoverytests/integration/api/v1/sessionstests/integration/api/v1/_use-casestests/integration/api/v1/usertests/integration/api/v1/users/[username]Tipo de mudança
Checklist:
Atividades necessárias "Pré-deploy"
Observação: é minha primeira contribuição em um projeto open source, então se algo não ficou implementado da melhor forma me avisem para que possa ajustar, principalmente nas alterações de interface 😅.