Node-Registrar review

[xmonader]

main.go

this leaks the logic of the server into a place where it's whole responsibility is doing

if err:=server.Run(); err!=nil {

  ...
}

It shouldn't bother about the sighandlers or anything else

• validation for sqllog level?

• validation for adminTwinID?that could never be zero, while it's set as a default value?
• add timestamps to the console logger

db.go

• 

Condition is wrong, should be ||, it's correctly checked in the cmd/main.go

• PostgresHost
  

check should be if err!=nil

db/models.go

there's no mechanism to set node to Approved (should allow the farmer to approve the node to be part of their farm)

util.go

Does nothing but verify signature, rename to sigverifier.go

const PubKeySize = 32 // ED25519/SR25519 public key size

var (
    ErrInvalidInput    = errors.New("invalid input")
    ErrVerifyFailed    = errors.New("signature verification failed")
)

• need to extract the pubkey size into a const and also extract the static errors
• do validation on challenge, signature e.g != 0 before doing anything even
• you can avoid the dupication e.g

func verifyWithScheme(scheme interface{}, publicKey, challenge, signature []byte) bool {
    key, err := scheme.FromPublicKey(publicKey)
    if err != nil {
        return false
    }
    return key.Verify(challenge, signature)
}

server.go

• there's no error handling
• there's a leak of internals into main.go as mentioned above
• for the Sthudown use a context with a Timeout
• need to handle http.ErrServerClosed in ListenAndServe

server/auth middleware

• reduce the window size to 1 minute
• could be a good idea to include a nonce and track it to prevent more replay attacks (later on that)
• log the exact error on the server side and return generic error in the response e.g

log.Warn().Err(err).Msg("public key decode failed")
abortWithError(c, http.StatusBadRequest, "invalid account configuration")

• time skew (can be addressed later when we settle on the challenge validity, as it's right now valid for 5mins or will be for 1 after the modification)

const clockTolerance = 5 * time.Second
if time.Since(time.Unix(timestamp, 0)) > (ChallengeValidity + clockTolerance) {

• use a unique context key

• also remove the todo, given that we support ed and sr now no?

routes

• change updateFarmsHandler -> updateFarmHandler
• make also the api to be served on /api/v1, right now that's only /v1
• please don't depend on the order of router registration before and after the middleware

// Farms
publicFarmRoutes := v1.Group("farms")
publicFarmRoutes.GET("/", s.listFarmsHandler)
publicFarmRoutes.GET("/:farm_id", s.getFarmHandler)

protectedFarmRoutes := v1.Group("farms", s.AuthMiddleware())
protectedFarmRoutes.POST("/", s.createFarmHandler)
protectedFarmRoutes.PATCH("/:farm_id", s.updateFarmsHandler)

handlers.go

return after sending error in the response, because that will incorrectly send 200 ok

check errors with Is or As not ==

return after sending the json

name updateFarmsHandler => updateFarmHandler

createFarm, registerNode are missing validations e.g twin ID or farmID

• remove the commented functions e.g

• isValidPublicKey only checks on length not format?

• status codes not optimal e.g this should be an internal server error not a bad request

• logging request.Body without copying?
  

[Eslam-Nawara]

WIP
Addressed most of the points here, but few are left will be addressed separately

• adding mechanism to set node to Approved (should allow the farmer to approve the node to be part of their farm) add mechanism to set node to Approved #47
• include a nonce and track it to prevent more replay attacks